View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0004474 | Slicer4 | Module DICOM | public | 2017-11-09 10:06 | 2017-11-27 09:48 |
Reporter | alexis.girault | Assigned To | pinter | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | Slicer 4.8.1 | ||||
Target Version | Slicer 4.9.0 | Fixed in Version | Slicer 4.9.0 | ||
Summary | 0004474: TypeError in DICOMUtils | ||||
Description | if DICOMUtils.loadSeriesByUID input is not a LIST of VALID UIDS, then we get:
Which means it will not fail gracefully in the following cases:
| ||||
Steps To Reproduce | In python interactor:
| ||||
Additional Information | Regarding the second case:
| ||||
Tags | No tags attached. | ||||
Thanks! Good timing, I'm just editing this file. I'll do this right now. |
|
I saw you wrote those Utils a while back (thanks!) and that you were currently touching them up. Do you want me to create a separate issue for the 'Additional Information' (load patient by UID and not by index) or not? |
|
PatientUID means the patient UID in the CTK DICOM database (and not the DICOM tags, in which there is no PatientUID, but PatientID), and is an incremental integer. So I think the issue you explained in Additional Information is not something to be fixed. The documentation of that function says "Load patient by patient UID from DICOM database", which is consistent with how it works and what to expect. I'll fix the TypeError failures |
|
I expected the contrary: with See pictures attached:
|
|
There is no "PatientUID" in DICOM. There is a PatientID, which is different. PatientUID refers to the ID called by this name in the CTK DICOM database, which is an incremental integer. |
|
After my fixes this is the output. Let me know if it's OK. Note that the error in the last call comes from DICOMWidgets, so it's outside the scope of my current work (and this issue based on its title): |
|
I understand that. However:
The only reason I bring this up is that both Series and Patients have the same DICOM UID field shown in the information window, and loading with the first one works while loading with the later doesn't. Don't you think that is a legit source of confusion? IMO that shows there is a need for at least adding a function to load by "Patient ID", as well as renaming some functions or tags that are displayed around the DICOM browser and the data module, for consistency.
Could this return True or False like the other functions?
Nitpick: do you think it would make sense to have an error message in
It seems the error message is missing a whitespace between "UID" and the index ("None", "invalid_patient_index")
Nitpick: would instantiating the list within
No problem, thanks a lot for your help. However, the issue is about the |
|
PatientID-PatientUID confusion: I see your point, and the source of the confusion. However, these convenience functions were implemented to mirror the way the ctkDICOMDatabase works (which also should have many more convenience functions), and I added the loadPatientByName function as an extra convenience. If you want to load by PatientID, feel free to add another convenience function and issue a PR.
Certainly, done
Sure
Already fixed, forgot to update screenshot
I decided against it, as it can be any type of object, and wrapping it in a list would not necessarily make it better. Further, these utility functions are intended to be reasonably foolproof, and I don't necessarily want it to automatically fix all kinds of imaginable errors based on heuristics. If you get this error message, you can easily wrap your variable between square brackets in literally a second. |
|
Thanks Csaba Few suggestions:
EDITED: |
|
What's uuid? Also what should we call the ctkDICOMDatabase PatientUID? I think the confusion stems from the fact that the names of the two are almost identical (PatientUID vs PatientID, and DICOM has UIDs everywhere except this one, so it is super easy to overlook) |
|
The following snippet returns the DICOM PatientID and not the ctkDICOMDatabase PatientUID, but the function is called |
|
Ooops. This is typo. |
|
Oh, OK! I can make such a function. Already started to do this. I'll also add the function that gets the patient UID by PatientID. I made some changes in function names to decrease ambiguity.
Good point! I don't have a good answer for this. The "DICOMUID" means different things for the SH items based on what level of the DICOM hierarchy it sits. For patients it is PatientID (due to lack of a proper ID, this is kind of a handwavy part of DICOM), for studies it is StudyInstanceUID, and for series it is SeriesInstanceUID (the latter are proper UIDs). @alexis.girault |
|
PR updated, every actionable in this issue should be addressed by now. |
|
Would calling this 'PatientNumber' help? Just a suggestion, if you think it sticks.
Thanks! |
|
Maybe patient number is a good choice, I also considered that. The problem with any renaming is that then we would be in discrepancy with CTK. I think everybody who deals with DICOM needs to know that PatientID is a strange tag that does not uniquel identify a patient, only with PatientName and PatientBirthDate. We also need to keep in mind that CTK refers to the patients in its database with UIDs, which is different. I can to explain this to people who get confused :) I set the ticket to resolved. Please set it to closed when the PR is integrated (and you consider it an acceptable solution). If we want to open a discussion about nomenclature then I think we need to involve others too (Steve, Andrey, Andras) and should do it on discourse. |
|
The commit https://github.com/Slicer/Slicer/commit/fce63b47161baecc320672e93b879c0e08d3ed9a adds the features and fixes the bugs requested in this issue (a subsequent fix by Christian Herz https://github.com/Slicer/Slicer/commit/39d125d51fe074a986afb56660603de5edceb5ad). Alexis, please close this if you're satisfied, or let me know what else to do if not yet. Thanks! |
|
LGTM, thanks. |
|
Date Modified | Username | Field | Change |
---|---|---|---|
2017-11-09 10:06 | alexis.girault | New Issue | |
2017-11-09 10:06 | alexis.girault | Status | new => assigned |
2017-11-09 10:06 | alexis.girault | Assigned To | => pieper |
2017-11-09 10:07 | alexis.girault | Description Updated | View Revisions |
2017-11-09 10:07 | alexis.girault | Steps to Reproduce Updated | View Revisions |
2017-11-09 10:07 | alexis.girault | Additional Information Updated | View Revisions |
2017-11-09 10:08 | alexis.girault | Additional Information Updated | View Revisions |
2017-11-09 14:55 | alexis.girault | Assigned To | pieper => pinter |
2017-11-09 14:58 | pinter | Note Added: 0015389 | |
2017-11-09 15:03 | alexis.girault | Note Added: 0015390 | |
2017-11-09 15:08 | pinter | Note Added: 0015391 | |
2017-11-09 15:08 | pinter | Note Edited: 0015391 | View Revisions |
2017-11-09 15:18 | alexis.girault | File Added: Screen Shot 2017-11-09 at 3.15.44 PM.png | |
2017-11-09 15:18 | alexis.girault | File Added: Screen Shot 2017-11-09 at 3.16.01 PM.png | |
2017-11-09 15:18 | alexis.girault | Note Added: 0015392 | |
2017-11-09 15:22 | pinter | Note Added: 0015393 | |
2017-11-09 15:23 | alexis.girault | Note Edited: 0015392 | View Revisions |
2017-11-09 15:49 | pinter | Note Added: 0015394 | |
2017-11-09 15:50 | pinter | Note Edited: 0015394 | View Revisions |
2017-11-09 15:51 | pinter | Note Edited: 0015394 | View Revisions |
2017-11-09 15:52 | pinter | File Added: 20171109_DICOMUtilsOutput.PNG | |
2017-11-09 16:09 | alexis.girault | Note Added: 0015395 | |
2017-11-09 16:10 | alexis.girault | Note Edited: 0015395 | View Revisions |
2017-11-09 16:21 | pinter | Note Added: 0015396 | |
2017-11-09 16:21 | pinter | Note Edited: 0015396 | View Revisions |
2017-11-09 16:21 | pinter | Note Edited: 0015396 | View Revisions |
2017-11-09 19:11 | jcfr | Assigned To | pinter => jcfr |
2017-11-09 19:12 | jcfr | Assigned To | jcfr => pinter |
2017-11-09 19:19 | jcfr | Note Added: 0015397 | |
2017-11-10 08:43 | pinter | Note Added: 0015398 | |
2017-11-10 09:33 | alexis.girault | Note Added: 0015399 | |
2017-11-10 09:33 | jcfr | Note Added: 0015400 | |
2017-11-10 09:34 | jcfr | Note Edited: 0015397 | View Revisions |
2017-11-10 09:34 | jcfr | Note Edited: 0015397 | View Revisions |
2017-11-10 09:36 | alexis.girault | Note Edited: 0015399 | View Revisions |
2017-11-10 10:08 | pinter | Note Added: 0015401 | |
2017-11-10 10:37 | pinter | Note Added: 0015402 | |
2017-11-10 14:22 | alexis.girault | Note Added: 0015403 | |
2017-11-10 14:26 | pinter | Status | assigned => resolved |
2017-11-10 14:26 | pinter | Resolution | open => fixed |
2017-11-10 14:26 | pinter | Fixed in Version | => Slicer 4.9.0 |
2017-11-10 14:26 | pinter | Note Added: 0015404 | |
2017-11-23 10:52 | pinter | Note Added: 0015438 | |
2017-11-27 09:47 | alexis.girault | Note Added: 0015445 | |
2017-11-27 09:48 | alexis.girault | Status | resolved => closed |