Skip to content

Stop adding extraneous metadata padding #593

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

Merged
merged 2 commits into from
Feb 7, 2018

Conversation

jstutters
Copy link
Contributor

If the metadata field is already aligned to a 16-byte boundary then
don't add additional padding. This allows the user to create a
16-byte aligned extension padded with 0x20 that can be read by the Jim
software
(which doesn't like finding 0x00 in the metadata chunk).

If the metadata field is already aligned to a 16-byte boundary then
don't add a additional padding.  This allows the user to create a
16-byte aligned extension padded with 0x20 that can be read by the Jim
software (which doesn't like finding 0x00 in the metadata chunk).
@coveralls
Copy link

coveralls commented Feb 5, 2018

Coverage Status

Coverage decreased (-1.03%) to 95.333% when pulling dbfb71d on jstutters:avoid-extra-padding into ee4ce2a on nipy:master.

@codecov-io
Copy link

codecov-io commented Feb 5, 2018

Codecov Report

Merging #593 into master will decrease coverage by 1.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #593      +/-   ##
=========================================
- Coverage   94.46%   93.4%   -1.06%     
=========================================
  Files         177     189      +12     
  Lines       24936   25820     +884     
  Branches     2659    2752      +93     
=========================================
+ Hits        23556   24118     +562     
- Misses        908    1205     +297     
- Partials      472     497      +25
Impacted Files Coverage Δ
nibabel/tests/test_nifti1.py 98.38% <100%> (ø) ⬆️
nibabel/nifti1.py 91.15% <100%> (+0.01%) ⬆️
nibabel/externals/__init__.py 100% <0%> (ø)
nibabel/externals/netcdf.py 83.91% <0%> (ø)
nibabel/benchmarks/bench_finite_range.py 26.92% <0%> (ø)
nibabel/externals/tests/test_six.py 76.47% <0%> (ø)
nibabel/benchmarks/bench_arrayproxy_slicing.py 21.56% <0%> (ø)
nibabel/benchmarks/bench_load_save.py 18.18% <0%> (ø)
nibabel/benchmarks/bench_streamlines.py 21.56% <0%> (ø)
nibabel/externals/six.py 100% <0%> (ø)
... and 4 more

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 ee4ce2a...dbfb71d. Read the comment docs.

@effigies
Copy link
Member

effigies commented Feb 5, 2018

Good catch. Any chance of writing a test for this case?

@jstutters
Copy link
Contributor Author

@effigies I've extended test_nifti1/test_extension_basics to cover this situation. Unfortunately coveralls is still showing that as a drop in coverage but both code paths are executed (unless I've missed something).

@effigies
Copy link
Member

effigies commented Feb 6, 2018

The coverage checks can be a little weird at times. The main one I pay attention to is the diff.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Anybody else have comments?

For a small change like this, I'll merge in 24 hours unless someone objects.

@matthew-brett
Copy link
Member

Looks good to me too - thanks for the spot ...

@effigies effigies merged commit 6eacaf5 into nipy:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants