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

Support boolean values #467

Merged
merged 8 commits into from
Oct 20, 2022
Merged

Support boolean values #467

merged 8 commits into from
Oct 20, 2022

Conversation

lawrence-mbf
Copy link
Collaborator

Motivation

h5py boolean values were not supported correctly.

How to test the behavior?

See #466 (comment)

This PR only fixes a subset of issues when reading this file.

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

@lawrence-mbf lawrence-mbf self-assigned this Oct 5, 2022
Now returns table objects which match the default io.parseCompound function.
reorder long lines, remove useless comments.
@lawrence-mbf lawrence-mbf marked this pull request as ready for review October 5, 2022 19:05
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #467 (75e9dde) into master (9aa5613) will increase coverage by 0.02%.
The diff coverage is 77.56%.

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
+ Coverage   87.53%   87.56%   +0.02%     
==========================================
  Files         127      128       +1     
  Lines        5247     5348     +101     
==========================================
+ Hits         4593     4683      +90     
- Misses        654      665      +11     
Impacted Files Coverage Δ
+io/getMatTypeSize.m 88.88% <ø> (ø)
+io/parseDataset.m 85.18% <42.85%> (-8.57%) ⬇️
+io/writeCompound.m 77.33% <42.85%> (-1.84%) ⬇️
+io/getMatType.m 52.63% <50.00%> (+37.92%) ⬆️
+io/parseCompound.m 76.92% <53.84%> (-4.90%) ⬇️
+io/isBool.m 62.50% <62.50%> (ø)
+io/parseAttributes.m 86.95% <77.77%> (-7.64%) ⬇️
+types/+untyped/DataStub.m 95.04% <95.23%> (+0.28%) ⬆️
+tests/+unit/boolTest.m 95.83% <95.83%> (ø)
+io/getBaseType.m 88.88% <100.00%> (+0.65%) ⬆️
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bendichter
Copy link
Contributor

@lawrence-mbf this looks good to me! Do you know what lines of code are untested and how they might be tested? I can see that you added a test that does I/O for bool, but I'm not crazy about lowering our test coverage numbers if we can avoid it

@lawrence-mbf
Copy link
Collaborator Author

@bendichter I'll take a look at it. I think some of it is because of certain branches in getRow being added in.

The branch is still there so for coverage it's probably best to test it.
We need to cheat a little with this change as it is now guaranteed that reading compound data
types returns tables instead of structs with vectors.
Error message moved into addRow function.
@lawrence-mbf
Copy link
Collaborator Author

@bendichter coverage improved

@CodyCBakerPhD
Copy link
Collaborator

Heya, this is needed for catalystneuro/ndx-extract#4 and other things around the ecosystem, I'm sure. Anything left to do here?

@lawrence-mbf
Copy link
Collaborator Author

@CodyCBakerPhD Just eyes. All behavior tests are passing.

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Oct 20, 2022

When used with catalystneuro/ndx-extract#4 creates values appearing like

image

in HDFView, and when using h5py they are read back as np.bool_, so looks good to me!

@bendichter Any last thoughts?

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.

3 participants