-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update BatchStructureSetConversion.py #141
base: master
Are you sure you want to change the base?
Conversation
change the name of saved file to patient ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I added some suggestions.
if not os.access(output_dir, os.F_OK): | ||
os.mkdir(output_dir) | ||
save_rtslices(output_dir, use_ref_image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the argument back, this would break the reference image feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between "save_rtslices(output_dir, use_ref_image)" and "save_rtslices(output_dir)". I know this is a previous issue, but after reading the post, I can't realize what is reference image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you specify a reference image, then the saved NRRDs will have that dimension instead of the segments being cropped to their "effective extent". But even if you don't understand a feature, removing it is not a good idea :)
slicer.mrmlScene.Clear(0) # clear the scene | ||
DICOMUtils.loadPatientByUID(patient) | ||
output_dir = os.path.join(output_folder,patient) | ||
slicer.mrmlScene.Clear(0) # clear the scen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You introduced a typo here
DICOMUtils.loadPatientByUID(patient) | ||
output_dir = os.path.join(output_folder,patient) | ||
slicer.mrmlScene.Clear(0) # clear the scen | ||
studyList=db.studiesForPatient(patient) # code modified from https://www.slicer.org/wiki/Documentation/Nightly/ScriptRepository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more complicated than necessary. I think this would work better:
slicer.dicomDatabase.fieldForPatient('PatientID', patient)
In addition, I think the database UID should be kept, because in DICOM the patient ID is not necessarily unique, and it is unique together with the patient name, but it is fragile to use patient name in path. With the database patient UID though, it is unique within the Slicer database. Something like this:
patientID = slicer.dicomDatabase.fieldForPatient('PatientID', patient)
folderName = str(patient) + '_' + patientID
@lassoan What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patient ID can be really anything: it is often set to empty during image anonymization (even though it is not valid), can be too long to be used as filename, and may contain special characters that cannot be used in filenames. So, they are definitely much more risky to use than the database's patient UID.
I would recommend to make addint patient name+id to folder name optional and sanitize the string (remove special characters, limit maximum size, etc.).
change the name of saved file to patient ID