-
Notifications
You must be signed in to change notification settings - Fork 429
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
Bugfix: Horizon image's dtype validation #3031
Bugfix: Horizon image's dtype validation #3031
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3031 +/- ##
==========================================
+ Coverage 82.08% 82.11% +0.02%
==========================================
Files 146 146
Lines 20467 20495 +28
Branches 3261 3268 +7
==========================================
+ Hits 16800 16829 +29
+ Misses 2856 2855 -1
Partials 811 811
|
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.
Hi @maharshi-gor,
Looks good to me. See below a very small comment but this PR is ready to be merged.
Thanks!
dipy/viz/tests/test_apps.py
Outdated
def check_for_warnings(warn_printed, w_msg): | ||
selected_w = [w for w in warn_printed if issubclass(w.category, | ||
UserWarning)] | ||
assert len(selected_w) >= 1 | ||
msg = [str(m.message) for m in selected_w] | ||
npt.assert_equal(w_msg in msg, True) |
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.
this function is duplicated.
Maybe you should moved this function in dipy.testing.__init__.py
dipy/viz/tests/test_util.py
Outdated
def check_for_warnings(warn_printed, w_msg): | ||
selected_w = [w for w in warn_printed if issubclass(w.category, | ||
UserWarning)] | ||
assert len(selected_w) >= 1 | ||
msg = [str(m.message) for m in selected_w] | ||
npt.assert_equal(w_msg in msg, True) |
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.
duplicated, to move somewhere else
dipy/viz/horizon/util.py
Outdated
|
||
|
||
def check_img_dtype(images): | ||
"""Check for supported dtype. If not supported numerical type, fallback to |
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.
after the first .
you should return to a new line. see here for an example
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.
Thank you for this @maharshi-gor.
It looks good. merging
This PR resolves the issue #2789 about checking dtype of the passed image arrays.
Current behavior
Proposed behavior
All the images will be checked for dtype and the following cases can be the result.