Skip to content

Closes #1294 ge 3volumes flavor#1332

Merged
jan-petr merged 9 commits intodevelopfrom
bug-#1294_GE_3volumes_flavor
Apr 12, 2023
Merged

Closes #1294 ge 3volumes flavor#1332
jan-petr merged 9 commits intodevelopfrom
bug-#1294_GE_3volumes_flavor

Conversation

@HenkMutsaerts
Copy link
Member

@HenkMutsaerts HenkMutsaerts commented Mar 5, 2023

Linked issue

Closes #1294

@HenkMutsaerts HenkMutsaerts requested a review from jan-petr March 5, 2023 08:28
@HenkMutsaerts HenkMutsaerts self-assigned this Mar 5, 2023
@HenkMutsaerts HenkMutsaerts linked an issue Mar 5, 2023 that may be closed by this pull request
4 tasks
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The bugs are correctly fixed. Very good.
  2. The code checking for token and bracket numbers goes beyond what is the definition and will invalid also cases that are not incorrect - see further comments.
  3. The issue with cbf scan is not dealt with correctly. The correct solution is not to throw out volumes set as "dummies" but to recognize that when we have both control/label/m0scan etc and cbf, then the CBF should be thrown out. At least for GE, I would do that. The thing is that GE nicely works out of DICOM without extra settings and I would be happier if this is done automatically and automatically deciding to throw CBF out.

@HenkMutsaerts HenkMutsaerts force-pushed the bug-#1294_GE_3volumes_flavor branch from 1b6389f to 8c84b45 Compare March 14, 2023 17:50
@jan-petr jan-petr self-requested a review March 15, 2023 09:16
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last issue about token numbers is still not addressed.

@HenkMutsaerts
Copy link
Member Author

See my last edit, I have now added the clarification, that () is reserved only to define tokens (because otherwise, we don't know what is token or not in the interpretation, so we have to reinforce this with an error right?).

Unless you can come up with an example where you want to have () also for either/or, without making this illegal.
E.g., do you want to keep the possibility of doing instead of '(ASL|T1w)' '(ASL(01|02)|T1w)' ? Because '(ASL|T1w)(01|02)' would definitely not work (the 01 or 02 would become a token). In this situation, the user should just use '(ASL|T1w).*', because it otherwise is unclear, 01|02 could also be used for series? So "tokens within tokens" should be illegal, and () can only define tokens, right?

@jan-petr jan-petr self-requested a review April 12, 2023 17:14
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@jan-petr jan-petr force-pushed the bug-#1294_GE_3volumes_flavor branch from e08bd69 to fd3e5b7 Compare April 12, 2023 17:25
@jan-petr jan-petr merged commit fd3e5b7 into develop Apr 12, 2023
@jan-petr jan-petr deleted the bug-#1294_GE_3volumes_flavor branch April 12, 2023 17:25
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.

Minor GE flavor bugs

2 participants