Skip to content

Closes Bug #938 redundant dcm2nii behavior#939

Merged
MichaelStritt merged 5 commits intodevelopfrom
bug-#938_Redundant_dcm2nii_behavior
Nov 26, 2021
Merged

Closes Bug #938 redundant dcm2nii behavior#939
MichaelStritt merged 5 commits intodevelopfrom
bug-#938_Redundant_dcm2nii_behavior

Conversation

@HenkMutsaerts
Copy link
Member

@HenkMutsaerts HenkMutsaerts commented Nov 21, 2021

Linked issue

Closes #938

@HenkMutsaerts HenkMutsaerts linked an issue Nov 21, 2021 that may be closed by this pull request
@MichaelStritt MichaelStritt added the bug Something isn't working label Nov 21, 2021
@MichaelStritt MichaelStritt requested review from jan-petr and removed request for MichaelStritt November 21, 2021 09:07
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.

Looks good to me. Michael should approve as well and check if there won't be any indirect issues with moving the lines (I don't foresee anything).

@jan-petr jan-petr requested review from MichaelStritt and jan-petr and removed request for MichaelStritt November 25, 2021 13:22
@jan-petr
Copy link
Contributor

Many things need to be changed to make this work for PAR-REC again.

  1. Setting to version 2011 instead of 2019 cannot be done in settings because we don't know that we will have PAR/RECs - this doesn't need to be said in the sourcestructure.json. Maybe you do say it sometimes, but that's not a rule. So this decision has to be made before calling dcm2nii for each file.
  2. Inside xASL_io_dcm2nii, we first loaded option file based on the fact if we are treating PAR/REC and only then we run the code to recognize if we are handling PAR/REC. So this code had to be reversed.

@jan-petr
Copy link
Contributor

  1. The PARREC flavor is still not read correctly - it crashes on copying the two generated NIFTI files.

@MichaelStritt
Copy link
Contributor

Hm... cool that we don't check in three places of the workflow now, but if it doesn't work at all right now, we should definitely work on the actual bugfix. It also seems a bit risky just to check if file or folder is called PAR. We should probably restrict it to bMatchDirectories=false, so that it only works for DICOM files starting with PAR?

@MichaelStritt
Copy link
Contributor

The nicest solution would probably be if there was a specific DICOM tag for those datasets.

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.

All good.

Copy link
Member Author

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

Looks good

@MichaelStritt MichaelStritt force-pushed the bug-#938_Redundant_dcm2nii_behavior branch from 5e49e3e to 53dedef Compare November 26, 2021 12:10
@MichaelStritt MichaelStritt merged commit 53dedef into develop Nov 26, 2021
@MichaelStritt MichaelStritt deleted the bug-#938_Redundant_dcm2nii_behavior branch November 26, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redundant dcm2nii_version behavior

3 participants