TST: Validate nib-diff dtype command-line option #672

chrispycheng · 2018-10-03T17:45:08Z

This PR is based on PR #661
It should now be possible to modify the datatype used to compare the differences as seen below:

$> nib-diff --dt float32 nibabel/tests/data/standard.nii.gz nibabel/tests/data/example4d.nii.gz 
These files are different.
Field/File     1:standard.nii.gz                                      2:example4d.nii.gz                                     
regular        b''                                                    b'r'                                                   
dim_info       0                                                      57                                                     
dim            [3 4 5 7 1 1 1 1]                                      [  4 128  96  24   2   1   1   1]                      
datatype       2                                                      4                                                      
bitpix         8                                                      16                                                     
pixdim         [ 1.  1.  3.  2.  1.  1.  1.  1.]                      [ -1.00000000e+00   2.00000000e+00   2.00000000e+00   2.19999909e+00
   2.00000000e+03   1.00000000e+00   1.00000000e+00   1.00000000e+00]
slice_end      0                                                      23                                                     
xyzt_units     0                                                      10                                                     
cal_max        0.0                                                    1162.0                                                 
descrip        b''                                                    b'FSL3.3\x00 v2.25 NIfTI-1 Single file format'         
qform_code     0                                                      1                                                      
sform_code     2                                                      1                                                      
quatern_b      0.0                                                    -1.9451068140294884e-26                                
quatern_c      0.0                                                    -0.9967085123062134                                    
quatern_d      0.0                                                    -0.0810687392950058                                    
qoffset_x      0.0                                                    117.8551025390625                                      
qoffset_y      0.0                                                    -35.72294235229492                                     
qoffset_z      0.0                                                    -7.248798370361328                                     
srow_x         [ 1.  0.  0.  0.]                                      [ -2.00000000e+00   6.71471565e-19   9.08102451e-18   1.17855103e+02]
srow_y         [ 0.  3.  0.  0.]                                      [ -6.71471565e-19   1.97371149e+00  -3.55528235e-01  -3.57229424e+01]
srow_z         [ 0.  0.  2.  0.]                                      [  8.25548089e-18   3.23207617e-01   2.17108178e+00  -7.24879837e+00]
DATA(md5)      0a2576dd6badbb25bfb3b12076df986b                       b0abbc492b4fd533b2c80d82570062cf                       
DATA(diff 1:)  -                                                      CMP: incompat

TODOs

Write tests for accessing dtype on cmdline
Get feedback on this implementation @yarikoptic @effigies

coveralls · 2018-10-03T18:13:14Z

Coverage remained the same at 91.84% when pulling edb72bc on chrispycheng:enh-diff-dtype into def0364 on nipy:master.

codecov-io · 2018-10-04T15:04:32Z

Codecov Report

Merging #672 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #672   +/-   ##
=======================================
  Coverage   88.86%   88.86%           
=======================================
  Files          93       93           
  Lines       11434    11434           
  Branches     1890     1890           
=======================================
  Hits        10161    10161           
  Misses        933      933           
  Partials      340      340

Impacted Files	Coverage Δ
nibabel/cmdline/diff.py	`93.75% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update def0364...edb72bc. Read the comment docs.

yarikoptic · 2018-10-06T03:01:29Z

nibabel/cmdline/diff.py

@@ -313,7 +320,8 @@ def main(args=None, out=None):
        files,
        header_fields=opts.header_fields,
        data_max_abs_diff=opts.data_max_abs_diff,
-        data_max_rel_diff=opts.data_max_rel_diff
+        data_max_rel_diff=opts.data_max_rel_diff,
+        dtype=opts.dtype


wouldn't work as is. In cmdline you will get a string, like "float64", whenever functions expect actual type, like np.float64

untested code is broken code :-P

Codecov appears green for it though? And I tested it in cmdline for myself. Could you please specify the use case you think I'm missing?

This is fine because whenever we get a dtype_var, we generally immediately turn around and run dtype(dtype_var), which will take np.float64, or '<f8' or 'float64'.

nibabel/nibabel/dataobj_images.py

Line 340 in 7877add

dtype = np.dtype(dtype)

still would be nice to have a test which invokes main here providing then dtype as a string.

Sounds reasonable to me.

Assuming these pass, please check to make sure this meets with your standards. I checked with @yarikoptic -- is this what you had in mind?

nibabel/tests/test_scripts.py

yarikoptic · 2018-10-11T16:18:49Z

ok, let me push a tiny change just to see the proper diff for this PR -- should be only dtype specification relevant now that we merged the other into master

…paces

yarikoptic · 2018-10-11T16:25:16Z

oh, I am not allowed to push to @chrispycheng branch. Do you have a check mark "checked" for "Allow changes from contributors" ... see e.g https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

effigies · 2018-10-11T17:04:23Z

Merging master in, it looks like this is a test/style update.

effigies · 2018-10-11T17:06:04Z

I'm good with this if y'all are.

yarikoptic · 2018-10-11T17:49:39Z

Thanks @effigies ... let's just travis to complete so nothing ugly creeps in

chrispycheng · 2018-10-11T22:51:19Z

Seems like everything's in order

yarikoptic reviewed Oct 6, 2018

View reviewed changes

storing changes for main testing

7315931

effigies reviewed Oct 11, 2018

View reviewed changes

nibabel/tests/test_scripts.py Outdated Show resolved Hide resolved

cmdline test added for dtype access

974a3a9

chrispycheng force-pushed the enh-diff-dtype branch from 8c8e6d8 to 974a3a9 Compare October 11, 2018 15:14

yarikoptic mentioned this pull request Oct 11, 2018

RF+ENH: nib-diff - allow to specify absolute and/or relative maximal diff to tolerate #661

Closed

4 tasks

ENH: adjusted some strings etc to fit the width and avoid duplicate s…

5361c7f

…paces

Merge branch 'master' into enh-diff-dtype

edb72bc

effigies changed the title ~~ENH: nib-diff - allow cmdline specification of which datatype to use to compare files' absolute and relative diff~~ TST: Validate nib-diff dtype command-line option Oct 11, 2018

yarikoptic merged commit 679aa5b into nipy:master Oct 12, 2018

TST: Validate nib-diff dtype command-line option #672

TST: Validate nib-diff dtype command-line option #672

Uh oh!

Conversation

chrispycheng commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yarikoptic Oct 6, 2018

Choose a reason for hiding this comment

Uh oh!

yarikoptic Oct 6, 2018

Choose a reason for hiding this comment

Uh oh!

chrispycheng Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

yarikoptic Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

chrispycheng Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yarikoptic commented Oct 11, 2018

Uh oh!

yarikoptic commented Oct 11, 2018

Uh oh!

effigies commented Oct 11, 2018

Uh oh!

effigies commented Oct 11, 2018

Uh oh!

yarikoptic commented Oct 11, 2018

Uh oh!

chrispycheng commented Oct 11, 2018

Uh oh!

Uh oh!

chrispycheng commented Oct 3, 2018 •

edited

Loading

coveralls commented Oct 3, 2018 •

edited

Loading

codecov-io commented Oct 4, 2018 •

edited

Loading