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

Replace file/name with hdf5 group/dataset when decoding #97

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

iguinn
Copy link
Contributor

@iguinn iguinn commented Jul 6, 2024

When calling read, the recursive functions will now pass the current group/dataset rather than a file/name pair. The file/name pair required re-looking up objects, which is quite slow because it involves re-parsing the hdf5 metadata.

Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 59.67742% with 25 lines in your changes missing coverage. Please review.

Project coverage is 74.28%. Comparing base (1317008) to head (b954b88).
Report is 67 commits behind head on main.

Files with missing lines Patch % Lines
src/lgdo/lh5/_serializers/read/composite.py 53.84% 6 Missing ⚠️
src/lgdo/lh5/core.py 40.00% 6 Missing ⚠️
...rc/lgdo/lh5/_serializers/read/vector_of_vectors.py 20.00% 4 Missing ⚠️
src/lgdo/lh5/_serializers/read/encoded.py 50.00% 2 Missing ⚠️
src/lgdo/lh5/_serializers/read/ndarray.py 75.00% 2 Missing ⚠️
src/lgdo/lh5/exceptions.py 33.33% 2 Missing ⚠️
src/lgdo/lh5/_serializers/read/array.py 88.88% 1 Missing ⚠️
src/lgdo/lh5/_serializers/read/scalar.py 83.33% 1 Missing ⚠️
src/lgdo/lh5/_serializers/read/utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   74.41%   74.28%   -0.14%     
==========================================
  Files          45       45              
  Lines        2806     2815       +9     
==========================================
+ Hits         2088     2091       +3     
- Misses        718      724       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gipert
Copy link
Member

gipert commented Jul 8, 2024

Wonderful Ian, this is exactly what I wanted to do. I'll marge and release a new version.

@gipert gipert merged commit 5fbe2f2 into legend-exp:main Jul 8, 2024
15 checks passed
@MoritzNeuberger
Copy link
Contributor

After this merge, I ran into issues loading the data by providing it with a list of file names.
I adjusted the code like such, and it seems to work again:

src/lgdo/lh5/core.py l. 117-120:

        for h5f in lh5_file:
            if isinstance(h5f, str):
                h5f = h5py.File(h5f, mode="r")  # noqa: PLW2901
            lh5_obj.append(h5f[name])

The issue seems to have been that the isinstance was checking lh5_file again instead of h5f.
Also, instead of += (extending the list), I used an append.

@iguinn
Copy link
Contributor Author

iguinn commented Jul 25, 2024

This is fixed (and there's a test now too): #101

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