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

DICOM: simplify handling of parsing in context of multi-frame #2311

Merged

Conversation

jdtournier
Copy link
Member

Changes to multiframe handling prompted by discussion on forum:
https://community.mrtrix.org/t/mrconvert-directions-in-0018-9089-not-detected/

The idea is to parse all tags regardless of which level in the hierarchy they appear in, rather than only parsing when they're top-level or within a Functional Group Sequence. Instead, we just need to ignore the IconImageSequence, which contains tags that can conflict with the main image; indeed, this seems to be the only reason the parsing worked as it did so far.

All DICOM tests pass fine with these changes.

Setting this up as a draft request for now as any changes to DICOM handling require quite a bit of double-checking...

maxpietsch and others added 8 commits October 26, 2020 15:34
When checking for matching gradient directions across reversed phase encoding acquisition in order to perform explicit volume recombination, do not rely on b=0 volumes having either nulled or equivalent gradient directions; instead, disable checking those directions if the shell to which both volumes are assigned is classified as b=0.
Use of broad Except clause resulted in obscuring AttributeError when testing for regeex matches. This meant that input strings that should ideally have been omitted due to an exclusion regex would nevertheless be included in the process list.
Changes to multiframe handling prompted by discussion on forum:
https://community.mrtrix.org/t/mrconvert-directions-in-0018-9089-not-detected/

The idea is to parse all tags regardless of which level in the hierarchy
they appear in, rather than only parsing when they're top-level or
within a Functional Group Sequence. Instead, we just need to ignore the
IconImageSequence, which contains tags that can conflict with the main
image; indeed, this seems to be the only reason the parsing worked as it
did so far.
@jdtournier jdtournier added the bug label Apr 21, 2021
@jdtournier jdtournier self-assigned this Apr 21, 2021
@jdtournier jdtournier added this to the 3.0.3 hotfix milestone Apr 21, 2021
@jdtournier jdtournier requested a review from a team April 21, 2021 12:34
@jdtournier jdtournier marked this pull request as ready for review June 9, 2021 07:31
@Lestropie Lestropie modified the milestones: 3.0.4 hotfix, 3.0.3 hotfix Jun 25, 2021
@Lestropie
Copy link
Member

Opened a bunch of random DICOMs across my file system and didn't find an issue; outstanding question is whether or not you're concerned about changes in the parsing of key-values, and whether or not you want to check for that in your tests.

@jdtournier
Copy link
Member Author

outstanding question is whether or not you're concerned about changes in the parsing of key-values, and whether or not you want to check for that in your tests.

Working on that as we speak. Lots of differences obviously, especially in the DW scheme as we're handling things slightly differently than previously, all of which requires careful double-checking... I'll hopefully have all this worked out by COB today. I'll be committing this to #2346 though. I'll merge this into #2346 so we can test everything in one go before merging.

@jdtournier jdtournier changed the base branch from master to dicom_dict_better_initialisation July 1, 2021 12:02
@jdtournier jdtournier merged commit f97ad9d into dicom_dict_better_initialisation Jul 1, 2021
@jdtournier jdtournier deleted the dicom_simplify_multiframe_handling branch July 1, 2021 12:30
@jdtournier jdtournier mentioned this pull request Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants