View Issue Details

IDProjectCategoryView StatusLast Update
0004474Slicer4Module DICOMpublic2017-11-27 09:48
Reporteralexis.girault Assigned Topinter  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product VersionSlicer 4.8.1 
Target VersionSlicer 4.9.0Fixed in VersionSlicer 4.9.0 
Summary0004474: TypeError in DICOMUtils
Description

if DICOMUtils.loadSeriesByUID input is not a LIST of VALID UIDS, then we get:

  File ".../qt-scripted-modules/DICOMLib/DICOMUtils.py", line 52, in loadSeriesByUID
    dicomWidget.detailsPopup.examineForLoading()
  File ".../qt-scripted-modules/DICOMLib/DICOMWidgets.py", line 661, in examineForLoading
    self.loadablesByPlugin, loadEnabled = self.getLoadablesFromFileLists(self.fileLists)
TypeError: 'NoneType' object is not iterable

Which means it will not fail gracefully in the following cases:

  1. If loadPatientByName input is not a name of a patient in the DICOM database
  2. If loadPatientByUID input is not an index of a patient in the DICOM database (between 1 and the number of patients in the database)
  3. If loadSeriesByUID input is a singleton (even valid UID)
  4. if loadSeriesByUID input list has only one item that is not a valid UID
Steps To Reproduce

In python interactor:

import DICOMLib as d
d.loadPatientByName("invalid_name")
d.loadPatientByUID("invalid_patient_index")
d.loadSeriesByUID("valid_serie_UID")
d.loadSeriesByUID(["invalid_serie_UID"])
Additional Information

Regarding the second case: loadPatientByUID input is not a UID, but the index of the patient in the DICOM database (between '1' and the number of patients in the list). That does not seem to be the expected behavior:

d.loadPatientByUID(1) # works (if at least one patient is in the database)
d.loadPatientByUID('valid_patient_UID') # fails with TypeError.
TagsNo tags attached.

Activities

pinter

pinter

2017-11-09 14:58

developer   ~0015389

Thanks! Good timing, I'm just editing this file. I'll do this right now.

alexis.girault

alexis.girault

2017-11-09 15:03

developer   ~0015390

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?

pinter

pinter

2017-11-09 15:08

developer   ~0015391

Last edited: 2017-11-09 15:08

View 2 revisions

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

alexis.girault

alexis.girault

2017-11-09 15:18

developer   ~0015392

Last edited: 2017-11-09 15:23

View 2 revisions

I expected the contrary: with loadStudiesByUID, the input is the UID from the DICOM tag, while using the same tag with the loadPatientByUID does not work, which isn't consistent. Also, there is no mention anywhere of an incremental patientUID in the DICOM browser, only of the PatientID (which really is the DICOM UID), so I find it confusing to have to load with an incremental index that is not displayed in there.

See pictures attached:

  1. Can load study with loadStudiesByUID(['1.3.12.2.1107.5.6.1.68562.30000016121521370051300000327'])
  2. Can not load patient with loadPatientByUID('04-150787')


pinter

pinter

2017-11-09 15:22

developer   ~0015393

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.

pinter

pinter

2017-11-09 15:49

developer   ~0015394

Last edited: 2017-11-09 15:51

View 3 revisions

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):
(formatting is messed up, the ">"s are treated as quotation, I'm adding a screenshot)

pinter

pinter

2017-11-09 15:52

developer  

alexis.girault

alexis.girault

2017-11-09 16:09

developer   ~0015395

Last edited: 2017-11-09 16:10

View 2 revisions

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.

I understand that. However:

  • That's inconsistent with what is shown in the Data module (subject hierarchy item information > UIDs: DICOM), which field can be used with loadSeriesByUID
  • The incremental integer is not shown anywhere in the DICOM Browser, while the PatientID (DICOM UID in subject hierarchy item) is
  • Being able to load by PatientId seems important since it is the actual unique identifier of a patient so it can stay consistent across any DICOM database. The incremental integer identifier changes if your database is updated, or if you are on another machine with a different database containing that patient (which really doesn't make it fit to be called "unique" per patient)

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.

d.loadPatientByName("invalid_name")

Could this return True or False like the other functions?

No patient found with DICOM database UIDNone

Nitpick: do you think it would make sense to have an error message in loadPatientByName if the UID that will be passed to loadPatientByUID is UIDNone that clearly says that the patient name was not found (instead of letting the next function say that UID "None" was not found)?

d.loadPatientByUID("invalid_patient_index")

It seems the error message is missing a whitespace between "UID" and the index ("None", "invalid_patient_index")

SeriesUIDs must be passed in a list

Nitpick: would instantiating the list within loadSeriesByUID and appending the input in it when the input is a singleton be a good idea? That would allow the input to be a singleton or a list.

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).

No problem, thanks a lot for your help. However, the issue is about the TypeError that is occurring when wrongfully calling DICOMUtils.loadSeriesByUID, so we should keep that ticket open until it is fixed in DICOMWidgets.

pinter

pinter

2017-11-09 16:21

developer   ~0015396

Last edited: 2017-11-09 16:21

View 3 revisions

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.

Could this return True or False like the other functions?

Certainly, done

Nitpick...

Sure

Whitespace

Already fixed, forgot to update screenshot

Nitpick#2

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.

jcfr

jcfr

2017-11-09 19:19

administrator   ~0015397

Last edited: 2017-11-10 09:34

View 3 revisions

Thanks Csaba

Few suggestions:

  • I suggest to also have a "verbose" keyword parameter, by default it could be True.

  • Instead of loadPatientByName, what about loadPatient(uid=None, name=None). We could then easily add other approach ...

EDITED: uuid -> uid

pinter

pinter

2017-11-10 08:43

developer   ~0015398

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)

alexis.girault

alexis.girault

2017-11-10 09:33

developer   ~0015399

Last edited: 2017-11-10 09:36

View 2 revisions

DICOM has UIDs everywhere except this one [patient], so it is super easy to overlook

The following snippet returns the DICOM PatientID and not the ctkDICOMDatabase PatientUID, but the function is called GetItemUID and the value is the DICOMUID tagged field. Does that mean we should change those names?

n = slicer.vtkMRMLSubjectHierarchyNode.GetSubjectHierarchyNode(slicer.mrmlScene)
dicom_uid_tag = slicer.vtkMRMLSubjectHierarchyConstants.GetDICOMUIDName() # = 'DICOM'
patientUID = n.GetItemUID(patientSubjectHierarchyItemId, dicom_uid_tag)
jcfr

jcfr

2017-11-10 09:33

administrator   ~0015400

Ooops. This is typo. uuid -> uid

pinter

pinter

2017-11-10 10:08

developer   ~0015401

This is typo. uuid -> uid

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.

The following snippet returns the DICOM PatientID and not the ctkDICOMDatabase PatientUID, but the function is called GetItemUID and the value is the DICOMUID tagged field. Does that mean we should change those names?

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).
In the ctkDICOMDatabase that incremental integer I referred to is called a UID, but it is not related to DICOM. I agree that this is quite confusing, but it is what it is mostly due to historical reasons (in DICOM and CTK). Renaming the function in SH I think is not an option, as that attribute contains a DICOM UID (the best we can have, which for patients is not really a UID). The best I can think of is to omit this attribute for patients. That would avoid confusion, but potentially make it harder to itendify a patient in the DICOM database.
Thoughts?

@alexis.girault
I made a fix for the last call of yours which I left in the screenshot above. Now it returns false and logs "Failed to offer loadables for DICOM series list" from the DICOMUtils layer, although the check is also done in DICOMWidgets, resulting in an error 'File lists must contain a non-empty list of strings'. You'll see it soon in PR https://github.com/Slicer/Slicer/pull/835

pinter

pinter

2017-11-10 10:37

developer   ~0015402

PR updated, every actionable in this issue should be addressed by now.

alexis.girault

alexis.girault

2017-11-10 14:22

developer   ~0015403

The best I can think of is to omit this attribute for patients. That would avoid confusion, but potentially make it harder to identifying a patient in the DICOM database.

Would calling this 'PatientNumber' help? Just a suggestion, if you think it sticks.

I made a fix for the last call of yours which I left in the screenshot above.

Thanks!

pinter

pinter

2017-11-10 14:26

developer   ~0015404

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.

pinter

pinter

2017-11-23 10:52

developer   ~0015438

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!

alexis.girault

alexis.girault

2017-11-27 09:47

developer   ~0015445

LGTM, thanks.

Issue History

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