RF+BF: Add tolerances and data types for nib-diff, remove hypothesis dependency #678
Conversation
…differences to tolerate
So now it should be possible to get an idea on how much data in the given files differs:
$> nib-diff --ma 0.000001 --mr .001 ./tests-run/output/./sub-1_T1w_5mm_noise_corrected.nii.gz /tmp/sub-1_T1w_5mm_noise_corrected.nii.gz
These files are different.
Field 1:sub-1_T1w_5mm_noise_corrected.nii.gz 2:sub-1_T1w_5mm_noise_corrected.nii.gz
DATA(md5) 65df09c06b236342eaf7e2fe57aabf55 3c6e9069e6e054e714f2894419848df0
DATA(diff 1:) - abs: 7.6293945e-06, rel: 0.002224694
…put, added tests for coverage
…ing script in test_scripts
…hanged value output in data_diff function to allow for validation of data type
Proved to give some troubles along the way and its use was quite basic and only in one tests module. So - replaced with simpler straightforward tests
There was a problem hiding this comment.
Didn't check for cases that didn't get covered (leaving that to @chrispycheng), but a couple minor style issues. Otherwise, LGTM.
nibabel/tests/test_diff.py
Outdated
| y = data.draw(st.floats(min_value=1e8), label='y') | ||
| z = data.draw(st.floats(max_value=-1e8), label='z') | ||
| def test_diff_values_int(): | ||
| long = 10**30 |
There was a problem hiding this comment.
long is a keyword in Python 2. Maybe large?
nibabel/cmdline/diff.py
Outdated
| return True | ||
| elif np.any(value0 != value): | ||
| return True | ||
| elif value0 is np.NaN: |
There was a problem hiding this comment.
Numpy prefers you to use np.nan. I don't know if this means they plan to remove the synonyms in the future or if it's just a style thing. I'd go with their recommendation, though.
Also appears in the test file.
Recommendataions by @effigies - long is a keyword - numpy.nan is recommended over numpy.NaN
|
@chrispycheng Are you happy with this PR? |
Codecov Report
@@ Coverage Diff @@
## master #678 +/- ##
==========================================
- Coverage 88.86% 88.83% -0.04%
==========================================
Files 93 93
Lines 11378 11434 +56
Branches 1869 1890 +21
==========================================
+ Hits 10111 10157 +46
- Misses 930 935 +5
- Partials 337 342 +5
Continue to review full report at Codecov.
|
|
Finally tests are green! |
|
@chrispycheng please look at https://codecov.io/gh/nipy/nibabel/pull/678/src/cmdline/diff.py and add those missing tests so we get all cases covered (ignore the uncovered reraising the exception case) |
|
@effigies I'm going to sort out the coverage Yarik mentioned and then we should be good to go |
…into no-hypothesis
chrispycheng
force-pushed
the
no-hypothesis
branch
from
5c5638a to
22f89e9
Compare
|
@yarikoptic @effigies Changes are fired up and ready to go with Travis passing and increased coverage. AppVeyor is causing trouble again but not due to anything from this commit. |
effigies
changed the title
|
@yarikoptic If you're happy with this, want to merge? |
|
Ok, let's do it! thanks @effigies and @chrispycheng |
As pointed by a few added tests, comparison wasn't doing proper job, especially in the light of comparing NaN values. For the purpose of this function we would like to assume that two NaNs are equal - we are comparing different headers so bringing the difference were both have NaNs is not desired.
Attn @chrispycheng - please review the changes. Since there were no tests pointing specifically what was the intention from the code comparing for NaNs, I had difficult time figuring out what we wanted to say.