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

Support complex-valued dwidenoising #679

Merged
merged 28 commits into from
Feb 29, 2024
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jan 24, 2024

Closes #677.

We can probably use ds004666 for testing/validation, though we'll need to add sidecar files with some metadata (PhaseEncodingDirection and TotalReadoutTime).

Changes proposed in this pull request

  • In collect_data, select "part-mag" or no part entity for "dwi" data.
    • This will ensure that regular DWI processing doesn't accidentally collect phase files.
  • Add steps to perform complex-valued dwidenoise when possible.
    • Limited to when (1) --denoise-method is set to dwidenoise, (2) --ignore doesn't include phase, (3) --denoise-after-combining is not enabled, and (4) phase data are available.
  • Add phase option to --ignore to explicitly disable complex-valued denoising when phase data are available.
  • In init_merge_and_denoise_wf, use layout.get_metadata() to get each DWI file's metadata.

qsiprep/workflows/dwi/merge.py Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented Jan 25, 2024

It seems like the "combining" step (not denoise_before_combining) involves processing (b0_threshold and not no_b0_harmonization) that would mess up the relationship between the magnitude and phase data, so we might need to limit complex dwidenoise to workflows with denoising before combining.

I also don't know what merging_distortion_groups really does. Does it just create a cross-run template, or does it do some initial processing to the DWI data before the denoising workflow?

EDIT: I just noticed that, in ds004666, not all runs and directions have phase data, so we can't take that for granted. Perhaps I should grab the phase data right before denoising, instead of within collect_data?

@tsalo
Copy link
Member Author

tsalo commented Jan 25, 2024

So what I ended up doing is passing along the BIDSLayout to init_merge_and_denoise_wf. Now it will try to find a phase file for each individual DWI file as it loops through the DWI files. Complex-valued denoising will only be allowed if denoise_before_combining is True, since I'm not sure if the extra steps from combining (e.g., the B0 harmonization) would mess up the relationship between the magnitude and phase data. Also not sure that we can assume that complementary magnitude runs will all have phase data.

@tsalo tsalo marked this pull request as ready for review January 25, 2024 18:14
@tsalo
Copy link
Member Author

tsalo commented Jan 25, 2024

Tests are now passing, so at least I didn't break the original workflows.

@tsalo
Copy link
Member Author

tsalo commented Feb 1, 2024

Tested on the 0p9mm session of ds004666, and it failed on MergeDWIs:

ValueError: MergeDWIs requires a value for input 'bval_files'. For a list of required inputs, see MergeDWIs.help()

EDIT: Ah, I see that this happened because ds004666 uses inheritance for the bval and bvec files.

@tsalo tsalo added the enhancement New feature or request label Feb 12, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a great idea - it doesn't require a new commandline flag and is easy to understand

qsiprep/interfaces/dwi_merge.py Show resolved Hide resolved
out_file = File(exists=True)


class PolarToComplex(CommandLine):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you remember where you found the mrcalc commands? Was it on the mrtrix forum?

Copy link
Member Author

Choose a reason for hiding this comment

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

qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
qsiprep/workflows/dwi/merge.py Show resolved Hide resolved
@mattcieslak mattcieslak merged commit a29109d into PennLINC:master Feb 29, 2024
3 checks passed
@tsalo tsalo deleted the phase branch March 4, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage phase data, when available, at denoising stage
2 participants