Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codecrazer
Copy link

change the name of saved file to patient ID

change the name of saved file to patient ID
Copy link
Member

@cpinter cpinter left a 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)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

Copy link
Member

@lassoan lassoan Apr 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants