-
-
Notifications
You must be signed in to change notification settings - Fork 23
fix val error when extracting recon size #353
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
Conversation
|
Hello again, there is second commit, this time related to proper merging of the slices within a single folder. This issue arises from use of e7tools, as it appears that e7tools modifies the DICOM headers for a few bins of the reconstruction and so the output of the dcm2niix4pet is multiple .nii files even though they are the same scan. If you would like to go about this another way instead of forcing dcm2niix to merge, please let me know and happy to contribute in a different way. Cheers, Rami |
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 making the PR! Since it seems to fixe your issue and doesn't appear to affect existing users (or at least the CI) this all seems fine to me. Can you bump the version by 0.0.1 in the pyproject.toml file? Or if you're not in a hurry to have these changes pushed to PyPI you could instead change the target branch to development instead of main and have these changes available now.
| # people use screwy paths, we do this before running dcm2niix to account for that | ||
| image_folder = helper_functions.sanitize_bad_path(self.image_folder) | ||
| cmd = f"{self.dcm2niix_path} -b y -w 1 -z y {file_format_args} -o {tempdir_pathlike} {image_folder}" | ||
| cmd = f"{self.dcm2niix_path} -b y -m Y -w 1 -z y -s y {file_format_args} -o {tempdir_pathlike} {image_folder}" |
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.
Okay, sure I think this is fine as it passes on the phantoms and the other CI. I'm still not certain what it is that -s is doing:
-s : single file mode, do not convert other images in folder (y/n, default n)
But again, it's not affecting our CI so I'm pretty okay with it.
|
@neurolabusc tagging you in as I'm not sure what the Going to ask @ramroomh if you are able to share the dicoms produced by e7 tools with us, or whether you can generate some with a siemens phantom from this public source https://datacatalog.publicneuro.eu/dataset/PN000001%20OpenNeuroPET%20Phantoms/V1 |
|
I suggest fixing the error in these images rather than treating the symptom. I certainly would not merge this change into code designed for robust use with valid DICOM images. I would caution against using |
|
Thank you @bendhouseart @neurolabusc. I agree what I have is a band-aid soultion and not optimal. As far as the Happy to share some reconstructions, thanks to @ltuominen lab for sharing. We have an XNAT instance for sharing and can add you to the relevant project containing the reconstructions. Also, we have a Siemens biograph mMR (PET/MR) and can ask to contribute a phantom from our site (The Royal, Ottawa) if you are interested @bendhouseart? I think there are two issues though:
|
|
Following the feedback from Chris and Rami I would say that:
I think this is fine, I'm all for leaving it.
For this specific use case I think a better fix is going to be to allow the input of additional arguments to dcm2niix either via the command line with a @ramroomh I'll try to make the changes directly to your branch, but if not I'm just going to implement them in the development branch of pet2bids so that they're available to you. |
|
Allowing additional input sounds like a great fix-- I agree it allows for flexibility. No worries about the branch, thanks for working on this @bendhouseart |
|
@neurolabusc if you are interested in some raclopride PET reconstructions with JSrecon (Siemens wrapper for e7tools), @ltuominen is happy to share for testing purposes. Let me know and I can add your email to our XNAT project page to download a few cases. |
|
@ramroomh I would be happy to see some examples. Feel free to share demos with my institutional email. |
|
Will do. Will close this as the fixes were addressed in #357. |
Hi @bendhouseart
Hope all is well
When using e7tools by Siemens, the ReconFilterSize field == 'GAUSSIAN 3 3', for example.
This produces the following error:
ValueError: could not convert string to float: '3 3'I've added a try/except statement to handle this specific case. I am assuming that any institute running e7tools would run into this problem!
Rami
📚 Documentation preview 📚: https://pet2bids--353.org.readthedocs.build/en/353/