FIX: Use simpler DWI reference workflow

[yibeichan]

I've made the change to replace the reference node based on #1125 (thanks @effigies for the instruction). Here is what I did:

1. copied this function into mriqc/workflows/diffusion/base.py, just above hmc_workflow, named as init_dmriref_wf
2. removed the dummy scan calculation in init_dmriref_wf, because it's not used anywhere in the current context
3. changed the docsting to be less BOLD focused.
4. modified the node dwi_reference_wf
5. changed the corresponding connections with dwi_reference_wf in the workflow

[pep8speaks]

Hello @yibeichan, Thank you for updating!

• In the file mriqc/workflows/diffusion/base.py:

Line 199:80: E501 line too long (84 > 79 characters)
Line 210:80: E501 line too long (86 > 79 characters)
Line 211:80: E501 line too long (81 > 79 characters)
Line 423:80: E501 line too long (88 > 79 characters)
Line 424:80: E501 line too long (83 > 79 characters)
Line 456:80: E501 line too long (94 > 79 characters)
Line 458:80: E501 line too long (83 > 79 characters)

To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2023-09-25 17:59:53 UTC

[effigies]

Overall looks good. The main thing is that unrelated changes need to be reverted.

[yibeichan]

thank you! I revised it based on your review. One question, what is this Workflow different from pe.Workflow? Because this Workflow was used in the fmriprep; I changed it to pe.Workflow here. Would it make any difference?
https://github.com/nipreps/fmriprep/blob/8d11614685d4522474944a055776d8f88ae56fe8/fmriprep/workflows/bold/reference.py#L25

from nipype.pipeline import engine as pe
from niworkflows.engine.workflows import LiterateWorkflow as Workflow

[effigies]

One question, what is this Workflow different from pe.Workflow?

fMRIPrep uses a subclass of nipype's Workflow which allows us to dynamically construct a description of what processing was applied to the dataset. That's what the __desc__ was about. MRIQC doesn't use that, so using pe.Workflow is correct.

[yibeichan]

aha, it should be good now. I think there was something in my vscode that automatically created those line breaks....

[oesteban]

Looking good - my comments intend to (i) calculate the average only with low b volumes, which is critical; and (ii) preempt realignment of the low b volumes for a double reason (ii.a) the low b are realigned at a later step and (ii.b) because we don't have really good parameters for 3dVolreg to work on DWIs.

[effigies]

@oesteban Looking over your suggestions, I think there were a couple mistakes. Here are some updated suggestions.

[yibeichan]

okay, made some updates based on the reviews

[effigies]

T1w test failure seems unrelated.

[welcome]

Thanks for opening this pull request and congratulations for taking it to the finish line! It looks like this is your first time contributing to MRIQC. 😄
We invite you to list yourself as an MRIQC contributor. To learn more about what that entails and how we credit our contributors, please check out the contributing guidelines. If your name is not already on the list, please insert it, in alphabetical order of (i) lastname and (ii) firstname, into the .maint/CONTRIBUTORS.md file.
Of course, if you want to opt-out this time, there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.