Skip to content
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

Allow SZIP compression options #167

Merged
merged 2 commits into from
Oct 9, 2019
Merged

Allow SZIP compression options #167

merged 2 commits into from
Oct 9, 2019

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Oct 8, 2019

Motivation

Fix #166
Enhance parameter checking in H5DataIO
Add tests for parameter checking

Checklist

  • Have you checked our Contributing document?
  • Have you ensured the PR description clearly describes problem and the solution?
  • Is your contribution compliant with our coding style ? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using #XXX notation where XXX is the issue number ? By including "Fix #XXX" you allow GitHub to close the corresponding issue.

ajtritt
ajtritt previously approved these changes Oct 8, 2019
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #167 into dev will increase coverage by 0.05%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #167      +/-   ##
==========================================
+ Coverage   69.91%   69.97%   +0.05%     
==========================================
  Files          30       30              
  Lines        5767     5785      +18     
  Branches     1355     1363       +8     
==========================================
+ Hits         4032     4048      +16     
  Misses       1302     1302              
- Partials      433      435       +2
Impacted Files Coverage Δ
src/hdmf/backends/hdf5/h5_utils.py 62.5% <89.65%> (+2.72%) ⬆️

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 bab66f3...8e8d221. Read the comment docs.

@oruebel
Copy link
Contributor Author

oruebel commented Oct 9, 2019

@rly @bendichter @ajtritt could you check and approve. I had to dismiss your approval to fix a bug that Ben had found.

@oruebel oruebel merged commit 8a6c650 into dev Oct 9, 2019
@oruebel oruebel deleted the enh/compszip branch October 9, 2019 18:33
Comment on lines +319 to +322
if not isinstance(self.__iosettings['compression_opts'], tuple) or \
len(self.__iosettings['compression_opts']) != 2:
raise ValueError("SZIP compression filter compression_opts" +
" must be a 2-tuple ('ec'|'nn', even integer 0-32")
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor, but regarding code style, I'm pretty sure the backslash and plus symbols are not necessary.

PEP8 says: "The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation."

Copy link
Contributor

Choose a reason for hiding this comment

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

You need an ending parenthesis inside the last quote marker on line 322

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we check for gzip compression opts being an int from 0-9, can we also check for the szip compression opts being ('ec'|'nn', even integer 0-32)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the issue with 322 is?

szip compression opts being ('ec'|'nn', even integer 0-32)?

sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a syntax error - but within the string, the "(" before "'ec'" needs a corresponding ")".

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.

szip compression parameters type checking fails in H5DATAIO
4 participants