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

ENH: Add number of volumes per *b*-value to QC report #150

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

josephmje
Copy link
Collaborator

@josephmje josephmje commented Jan 12, 2021

Closes #64 and #128. Replaces PR #73.

  • report diffusion sampling scheme: shelled, radial or Cartesian
  • report shell distribution
  • expose bmag parameter to adjust rounding bvals
  • check # b0s doesn't change after rounding
  • create tests for DiffusionSummary interface

In the case of DSI data, should round_bvals automatically round to the nearest 10 instead of calculating based on the magnitude of bvals? Set bmag = 1.

\t\t<details open>
\t\t<summary>Summary</summary>
\t\t<ul class="elem-desc">
\t\t\t<li>Distinct shells: {shells_dist}</li>
Copy link
Member

Choose a reason for hiding this comment

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

  • Why don't we also print out the shell distribution?
  • What happens with DSI schemes? Maybe we can add a unit test of the DiffusionSummary interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, shells_dist does print out the shell distribution. But perhaps I should give it a different label and variable name to make this more clear.

I've tested this on the taiwan_ntu_dsi dataset that comes bundled with dipy. Here's what it outputs:

{0: 7, 100: 12, 200: 8, 400: 6, 600: 24, 900: 24, 1500: 12, 1900: 30, 2400: 24, 2900: 24, 3400: 8, 4000: 24}

I haven't worked with DSI data before so I'm not quite sure if this is what we're looking for or whether this should be improved.

Will update with a unit test for the new interface!

@oesteban
Copy link
Member

Could you rebase when #151 is merged?

@josephmje
Copy link
Collaborator Author

I'll update this to see if I can figure out a parser that determines whether the data is shelled or sampled along the Cartesian grid. I've checked out the methods section of a few DSI papers and they seem to either report the maximum b value or a range (eg. x DWIs with diffusion weightings in the range of b = 100-4000 s/mm2 and x interleaved b = 0 scans). Perhaps a similar approach can be taken in our reportlet.

@oesteban
Copy link
Member

I'll update this to see if I can figure out a parser that determines whether the data is shelled or sampled along the Cartesian grid. I've checked out the methods section of a few DSI papers and they seem to either report the maximum b value or a range (eg. x DWIs with diffusion weightings in the range of b = 100-4000 s/mm2 and x interleaved b = 0 scans). Perhaps a similar approach can be taken in our reportlet.

Yes, this sounds fantastic.

@josephmje josephmje force-pushed the enh/count_shells branch 2 times, most recently from 86ebe84 to 1b8d632 Compare February 9, 2021 17:37
@pep8speaks
Copy link

pep8speaks commented Feb 9, 2021

Hello @josephmje, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2021-03-11 15:15:14 UTC

@josephmje josephmje force-pushed the enh/count_shells branch 2 times, most recently from 6a2fa08 to 4e6b3b9 Compare February 9, 2021 17:40
@josephmje
Copy link
Collaborator Author

This is ready for review. I will leave the DSI updates for another PR.

@josephmje
Copy link
Collaborator Author

@oesteban tests are failing after some of the changes to the dwi reference workflow. will wait until #153 gets merged and then will rebase

@oesteban
Copy link
Member

oesteban commented Mar 9, 2021

sounds great

@oesteban
Copy link
Member

should we resuscitate this?

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.

Determine number of shells
3 participants