-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add gradient_utils unit tests #903
Conversation
Hello @EmmaRenauld, Thank you for updating ! There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-02-20 13:39:03 UTC |
39a2d4e
to
34f04c6
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #903 +/- ##
==========================================
+ Coverage 65.95% 65.97% +0.01%
==========================================
Files 384 386 +2
Lines 21352 21573 +221
Branches 3495 3540 +45
==========================================
+ Hits 14083 14233 +150
- Misses 5661 5706 +45
- Partials 1608 1634 +26
|
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.
Looking good! I added a few comments that should be addressed before merging.
scilpy/gradients/utils.py
Outdated
@@ -3,10 +3,14 @@ | |||
import numpy as np | |||
|
|||
|
|||
def random_uniform_on_sphere(nb_vectors): | |||
def random_uniform_on_half_sphere(nb_vectors): |
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 don't think this produces points on the half sphere, but rather the whole sphere. I believe the previous name was right.
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.
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.
Not super well distributed, but not a half sphere
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.
GTG after the two typos are corrected and the name of the function is changed!
scilpy/gradients/utils.py
Outdated
Philips gradient table | ||
dwis: nibabel | ||
dwis | ||
ref_gradients_table: nd.array |
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.
ref_gradient_table, no s
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.
GTG
Quick description
Unit tests for the file gradients_utils.
As discussed, not sure about the random sphere thing... Went back to Caruyer's code, and it says on the half-sphere! Anyone understand why?
Type of change
Check the relevant options.
Provide data, screenshots, command line to test (if relevant)
...
Checklist