-
Notifications
You must be signed in to change notification settings - Fork 24
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] - extra bval parsing for number of shells #73
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! We have detected this is the first time for you to contribute to dMRIPrep. Please check out our contributing guidelines.
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. |
Best reviewed: commit by commit
Optimal code review plan (4 warnings)
|
Hello @edickie, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2020-04-07 22:57:59 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd reduce boilerplate and just use a function for this, but that's just an opinion :)
dmriprep/utils/vectors.py
Outdated
@@ -177,6 +179,105 @@ def to_filename(self, filename, filetype='rasb'): | |||
else: | |||
raise ValueError('Unknown filetype "%s"' % filetype) | |||
|
|||
class BVALScheme: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably go with a single function that accepts a DiffusionGradientTable
object for this.
The function would return only a list of nonzero shell centers and the number of shells would be just the len()
of that (which can be easily calculated by the user).
WDYT?
Do you think you can give this a little bump for tomorrow's meeting? |
Merging in updates from nipreps
Merging from upstream
Merging in updates from upstream master
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
- Coverage 51.06% 51.05% -0.02%
==========================================
Files 21 21
Lines 1218 1232 +14
Branches 161 164 +3
==========================================
+ Hits 622 629 +7
- Misses 581 590 +9
+ Partials 15 13 -2
Continue to review full report at Codecov.
|
I believe this from #29 is highly related to this PR. EDIT: I'll go through this PR tomorrow. Thanks for updating Erin! |
In response to issue #64 and discussion during the hack here is a little addition to the vectors utility that can determine and report on the number of the shells (as well as masks for each shell).
Currently, it is outside of DiffusionTable because I'm unsure if we want all the methods integrated..