ENH: Add gradient plot method

[jhlegarreta]

Add gradient plot method.

[jhlegarreta]

Cross reference nipreps/mriqc#1292 (comment).

Missing:

• Test data. I see that there is a tests/data folder, but no bval/bvec data exists. @effigies @oesteban can you please share some data that you can easily identify across the nipreps (data) repositories?
• There is no dependency on dipy, so reading the bval/bvec files will fail. I assume it is fair to add dipy as a dependency, right?
• An e.g. DWIGradientmap class. I guess it is this class that should contain the shelled vs. non-shelled data identification logic. IMO, this should be done in a separate PR.

[oesteban]

• Test data. I see that there is a tests/data folder, but no bval/bvec data exists. @effigies @oesteban can you please share some data that you can easily identify across the nipreps (data) repositories?

We could use bids examples or openneuro data.

We probably don't want to keep cloning and getting these files with git, so perhaps we can add in test/data:

• a single-shell, DTI-able example (https://github.com/bids-standard/bids-examples/blob/master/ds114/dwi.bvec)
• a multi-shell, DKI-able example (I can provide this)
• a DSI example (https://openneuro.org/datasets/ds004737/versions/1.0.0)

• There is no dependency on dipy, so reading the bval/bvec files will fail. I assume it is fair to add dipy as a dependency, right?

I'm fine adding dipy as a dependency, but I think here we want to assume the b-table comes in a particular format (e.g., FSL format, or RAS+b or IKJ+b). The user should just adapt their data before calling this.

• An e.g. DWIGradientmap class. I guess it is this class that should contain the shelled vs. non-shelled data identification logic. IMO, this should be done in a separate PR.

As we only plot one sphere, this is not a concern as far as I understand. That said, the previous apply -- let's define an API, as opposed to taking whatever comes and adapt it to work.

[oesteban]

I've given a gentle push to the testing side of things - let me know what you think @jhlegarreta

[codecov]

Codecov Report

Attention: Patch coverage is 95.16129% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 58.27%. Comparing base (0d893ff) to head (9f561d1).
Report is 6 commits behind head on main.

❗ Current head 9f561d1 differs from pull request most recent head 390f11d. Consider uploading reports for the commit 390f11d to get more accurate results

Files	Patch %	Lines
nireports/reportlets/modality/dwi.py	95.16%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   57.36%   58.27%   +0.91%     
==========================================
  Files          22       22              
  Lines        2024     2085      +61     
  Branches      359      321      -38     
==========================================
+ Hits         1161     1215      +54     
- Misses        782      787       +5     
- Partials       81       83       +2     

Flag	Coverage Δ
unittests	58.27% <95.16%> (+1.13%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[oesteban]

Tests are running, but the plots are weird: https://output.circle-artifacts.com/output/job/3d4b105c-09b1-492d-8ac4-4e95ea419715/artifacts/0/tmp/tests/output/ds000114_singleshell.svg

[jhlegarreta]

We could use bids examples or openneuro data.
We probably don't want to keep cloning and getting these files with git, so perhaps we can add in test/data:
a single-shell, DTI-able example (https://github.com/bids-standard/bids-examples/blob/master/ds114/dwi.bvec)
a multi-shell, DKI-able example (I can provide this)
a DSI example (https://openneuro.org/datasets/ds004737/versions/1.0.0)

Thanks for having added these.

I'm fine adding dipy as a dependency, but I think here we want to assume the b-table comes in a particular format (e.g., FSL format, or RAS+b or IKJ+b). The user should just adapt their data before calling this.

OK, I see what you did to read them.

As we only plot one sphere, this is not a concern as far as I understand. That said, the previous apply -- let's define an API, as opposed to taking whatever comes and adapt it to work.

OK. Not sure what you have in mind for the API, but I'd first go with one case (FSL format) see that things work, merge it, then deal with other cases that you think we should consider (RAS+b or IKJ+b).

I've given a gentle push to the testing side of things - let me know what you think @jhlegarreta

Looks good. Thanks for doing this.

Tests are running, but the plots are weird: https://output.circle-artifacts.com/output/job/3d4b105c-09b1-492d-8ac4-4e95ea419715/artifacts/0/tmp/tests/output/ds000114_singleshell.svg

I had tried with a pair of bval/bvec files from HCP (multi-shell), used the DIPY reader and provided the plots I had posted in nipreps/mriqc#1292, so maybe this whole thing had not been tested with single-shell and DSI when it was added to eddymotion. Will have a look as time permits.

[jhlegarreta]

Found out the issue:

gradients = np.hstack([bvecs[~b0s_mask], bvals[~b0s_mask, None]])

creates a RAS+b table, whereas the draw_points method expects data to be in FSL format:

gradients = np.vstack([bvecs[~b0s_mask].T, bvals[~b0s_mask]])

(Also found another issue with single shell data: fixed with the if rs.min() != rs.max(): condition: the inside block was giving NaNs as the denominator was 0 in that case).

I fixed the plot method to accept the RAS+b (honoring the docstring). Squashed all commits: sorry Óscar 🙏, looks like the squashed commit does not honor your co-authorship

Defining other possible layouts for the gradients (or a gradient API for the whole package) should be left for a separate PR.

[oesteban]

I fixed the plot method to accept the RAS+b (honoring the docstring). Squashed all commits: sorry Óscar 🙏, looks like the squashed commit does not honor your co-authorship

OMG, MY PRECIOUS COMMIT :_D

Found out the issue:

Good catch!

Thanks for this - I've enabled the tests (I'm also adding you as a maintainer)

[jhlegarreta]

For the sake of completeness, this is what the plots look like:

Single shell

Multi-shell

DSI

This is ready to be merged.

[oesteban]

Looks good -- just a few cosmetic changes. I'll merge