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 a shell data property to DWI data class #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Contributor

Add a shell data property to DWI data class that returns a list of pairs consisting of the estimated b-value and the associated DWI data.

Closes #66.

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jan 25, 2025

Opened as draft as I cannot debug this locally due to issue #73.

Edit: There seems to be some data formatting to be done to compare the DWI data obtained to the expected data, but the approach seems to be working otherwise:
https://github.com/nipreps/nifreeze/actions/runs/12969669089/job/36173994765?pr=74#step:12:1064
https://github.com/nipreps/nifreeze/pull/74/files#diff-f311d147ca2f3984f30068c37db4ca0a780e387581219cd6fa50a33e917d12cbR204

Edit: managed to debug locally undoing the type hinting related stuff. Ready to be reviewed.

@jhlegarreta jhlegarreta force-pushed the AddShellPropertyToDWIData branch 3 times, most recently from a516c84 to 3b62135 Compare January 25, 2025 22:30
Copy link

codecov bot commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.91%. Comparing base (9a143e4) to head (ad46aa3).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   68.78%   68.91%   +0.12%     
==========================================
  Files          20       20              
  Lines         958      962       +4     
  Branches      121      121              
==========================================
+ Hits          659      663       +4     
  Misses        254      254              
  Partials       45       45              

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

@jhlegarreta jhlegarreta force-pushed the AddShellPropertyToDWIData branch 5 times, most recently from eca3f42 to 7b7eb25 Compare January 26, 2025 00:20
@jhlegarreta
Copy link
Contributor Author

Maybe it is worthwhile thinking about the interplay between this feature and the b-value iterators, and how the whole thing fits into the estimators' pipelining philosophy.

@jhlegarreta jhlegarreta force-pushed the AddShellPropertyToDWIData branch 3 times, most recently from 2bbb863 to 5cd14cd Compare January 26, 2025 20:59
@jhlegarreta jhlegarreta marked this pull request as ready for review January 26, 2025 20:59
@jhlegarreta jhlegarreta force-pushed the AddShellPropertyToDWIData branch 2 times, most recently from 40e6519 to 20243e1 Compare January 27, 2025 15:11
Add a shell data property to `DWI` data class that returns a list of
pairs consisting of the estimated b-value and the associated DWI data.

Co-authored-by: Oscar Esteban <code@oscaresteban.es>
@jhlegarreta jhlegarreta force-pushed the AddShellPropertyToDWIData branch from 20243e1 to ad46aa3 Compare January 27, 2025 15:11
@oesteban
Copy link
Member

Maybe it is worthwhile thinking about the interplay between this feature and the b-value iterators, and how the whole thing fits into the estimators' pipelining philosophy.

I agree. In our last iteration we made the iterators independent of the object, so now you could go shell by shell and run any available iterator (those without concept of b-value, evidently).

Comment on lines +186 to +187
dwi_h5 = load(datadir / "dwi.h5")
num_bins = 3
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the bvecs/bvals files we added for testing? They are within the repo (i.e., no need to pull extra data from GIN) and they provide several use-cases ideal for testing 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.

Add a shells property to the DWI object to access the data split by shell
2 participants