WIP: parrec2nii sets qform code to 2

[JasperVanDenBosch]

fixes #463

[JasperVanDenBosch]

We've added two tests; the first, a unit test, that requires the mock library. (which is why it's currently failing). If this is not desirable, I'll take it out. If it is, where should I add the mock dependency, in requirements.txt or travis.yml?
The second test does a save->load type acceptance test.

[matthew-brett]

Thanks for doing this. Would you mind posting to the mailing list to ask for opinions about relying on the 'mock' library?

[matthew-brett]

Thanks. Would you mind adding mock to the dev-requirements.txt file as well. Could you grep for nose in the docs section, to update the installation / testing instructions?

[matthew-brett]

I added a check for "mock" when running tests via nibabel.test() : JasperVanDenBosch#1

[matthew-brett]

Use the machinery in test_scripts.py to find the executable?

[JasperVanDenBosch]

I completely overlooked that.. but the downside of that scriptrunner is that we still wouldn't be able to test the separate functions / import the code.

Long term I think ideally we'd refactor most of the logic out of the parrec2nii executable and into a regular module, which is more straightforward to test. For now I added a list of possible binary directories to look for parrec2nii.

[coveralls]

Coverage increased (+0.008%) to 96.251% when pulling e23dafc on ilogue:qform-provided-then-qform-code-2 into e37cb5d on nipy:master.

[coveralls]

Coverage increased (+0.008%) to 96.251% when pulling e23dafc on ilogue:qform-provided-then-qform-code-2 into e37cb5d on nipy:master.

[codecov-io]

Current coverage is 93.81% (diff: 51.26%)

Merging #478 into master will decrease coverage by 0.46%

@@             master       #478   diff @@
==========================================
  Files           161        163     +2   
  Lines         21321      21555   +234   
  Methods           0          0          
  Messages          0          0          
  Branches       2284       2315    +31   
==========================================
+ Hits          20100      20221   +121   
- Misses          801        902   +101   
- Partials        420        432    +12   

Powered by Codecov. Last update e37cb5d...aa184ed

[matthew-brett]

How about JasperVanDenBosch#2 ?

[JasperVanDenBosch]

Looks good, thanks! I'll check back if there's any failing tests.

[coveralls]

Coverage decreased (-0.4%) to 95.815% when pulling aa184ed on ilogue:qform-provided-then-qform-code-2 into e37cb5d on nipy:master.

[matthew-brett]

Thanks. I think coverage is down quite a bit, but I believe that's because the parrec2nii command code wasn't covered well, and we are now seeing that in the coverage report, so no not the fault of this PR.

Current PR is fine with me.

Any other suggestions?

[JasperVanDenBosch]

No further suggestions! Thanks

On Aug 11, 2016 2:49 PM, "Matthew Brett" [email protected] wrote:

Thanks. I think coverage is down quite a bit, but I believe that's because
the parrec2nii command code wasn't covered well, and we are now seeing that
in the coverage report, so no not the fault of this PR.

Current PR is fine with me.

Any other suggestions?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#478 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABcEjIWg2Nzz6Z3woX-H0cyLDQ2nSyfgks5qe5kDgaJpZM4JglbJ
.

[matthew-brett]

Actually, sorry, now I think if it - shouldn't the sform and qform codes both be 1 here? In that they indicate the scanner coordinates? @grlee77 - what do you think? Will this cause problems for you?

[mrjeffs]

hi matthew, because we are dealing with un-transformed data (pre-alignment with standard brains etc), both the q and sforms (and codes) should be the same whichever is decided. as i understand it, sform is diverged from qform when transformed into standard space and qform is kept to reference path to original subj space. see
https://github.com/stnava/ANTs/wiki/How-does-ANTs-handle-qform-and-sform-in-NIFTI-1-images%3F
Currently only sform is saved and sform code=2 in parrec2nii. see header dumps in my orig issues post https://github.com/nipy/nibabel/issues/463
you are right both codes should =1 if --origin='scanner' is default and no other transformation applied.
since arbitrarily sform code=2 when nibabel saves parrec images, does it change code from 1 to 2 when the parrec are transformed from Philips PLS to RAS+? technically that is a transformation by applying an affine but not an 'alignment'. this may be the default behavior since this exception has not been accounted for.
rather than ferret out this exception or have 2 different codes for the same space, i vote to manually set both sform and qform_code=1 e.g. 'Scanner Anat'. as in lines 220-226 in parrec2nii with my last 2 lines added:
# Make corresponding NIfTI image nimg = nifti1.Nifti1Image(in_data, affine, pr_hdr) nhdr = nimg.header nhdr.set_data_dtype(out_dtype) nhdr.set_slope_inter(slope, intercept) nhdr['qform_code'] = 1 nhdr['sform_code'] = 1
then both are saved identically and both =1.

[matthew-brett]

qform / sform code of 1 is fine with me, but it is a small API break. If there is some software that behaves differently with sform codes of 1 and 2 then something would change, but I doubt that would happen. OK to take the risk?

[grlee77]

I am fine with setting both to 1. (The previous behavior was sform=2, qform=0?)

[matthew-brett]

Right

[matthew-brett]

OK - I'll merge this as-is, then do a PR for the code changes.