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

Rework h5 metadata #2003

Merged
merged 9 commits into from
Jul 19, 2022
Merged

Rework h5 metadata #2003

merged 9 commits into from
Jul 19, 2022

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jul 11, 2022

This is a draft reworking how we deal with the hdf5 metadata.

  • To easily identify the "special" metadata fields, these now have a common CTAFIELD prefix
  • To follow the same style as the hdf5 table convention, the code now uses the integer position instead of the column name
  • As the prefix is stored in the metadata, the HDF5TableReader can now get it from the file metadata and does not have to be provided with prefixes if the mapping from Container field names to column names is unique, an error is raised otherwise.
  • The code creating the mapping from column names to container fields was simplified a lot, although it is now more powerful.

I still have to check performance and file size implications of this, and I would want to also address #1944.

This should fix #1971 and #1785

This breaks reading every file produced previously

@maxnoe maxnoe requested review from kosack, LukasNickel and nbiederbeck and removed request for kosack July 11, 2022 17:20
Copy link
Contributor

@nbiederbeck nbiederbeck left a comment

Choose a reason for hiding this comment

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

I really like this.

ctapipe/io/hdf5eventsource.py Outdated Show resolved Hide resolved
ctapipe/io/hdf5eventsource.py Outdated Show resolved Hide resolved
ctapipe/io/hdf5tableio.py Show resolved Hide resolved
ctapipe/io/tableio.py Outdated Show resolved Hide resolved
@nbiederbeck
Copy link
Contributor

This should also close #1969, I will check my case with this

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #2003 (46a5b3e) into master (93527bf) will increase coverage by 0.02%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##           master    #2003      +/-   ##
==========================================
+ Coverage   92.33%   92.35%   +0.02%     
==========================================
  Files         193      193              
  Lines       15708    15794      +86     
==========================================
+ Hits        14504    14587      +83     
- Misses       1204     1207       +3     
Impacted Files Coverage Δ
ctapipe/conftest.py 94.25% <ø> (ø)
ctapipe/io/datawriter.py 90.09% <ø> (ø)
ctapipe/io/hdf5eventsource.py 90.32% <92.30%> (+0.49%) ⬆️
ctapipe/io/astropy_helpers.py 87.96% <93.33%> (-2.52%) ⬇️
ctapipe/core/container.py 91.12% <100.00%> (+0.08%) ⬆️
ctapipe/io/hdf5tableio.py 95.84% <100.00%> (-0.31%) ⬇️
ctapipe/io/tableio.py 93.39% <100.00%> (ø)
ctapipe/io/tests/test_astropy_helpers.py 100.00% <100.00%> (ø)
ctapipe/io/tests/test_datawriter.py 100.00% <100.00%> (ø)
ctapipe/io/tests/test_hdf5.py 99.66% <100.00%> (+0.03%) ⬆️
... and 2 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 93527bf...46a5b3e. Read the comment docs.

@maxnoe maxnoe force-pushed the rework_h5_metadata branch from a0cb235 to 9c8062c Compare July 12, 2022 12:35
@maxnoe maxnoe requested a review from nbiederbeck July 12, 2022 12:36
nbiederbeck
nbiederbeck previously approved these changes Jul 12, 2022
nbiederbeck
nbiederbeck previously approved these changes Jul 13, 2022
@maxnoe maxnoe marked this pull request as ready for review July 14, 2022 08:08
@maxnoe maxnoe force-pushed the rework_h5_metadata branch from b20ee6c to c136133 Compare July 14, 2022 08:09
nbiederbeck
nbiederbeck previously approved these changes Jul 15, 2022
@maxnoe maxnoe requested a review from nbiederbeck July 19, 2022 09:26
@maxnoe maxnoe merged commit 4793557 into master Jul 19, 2022
@maxnoe maxnoe deleted the rework_h5_metadata branch July 19, 2022 09:32
@maxnoe maxnoe linked an issue Jul 19, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store used container prefixes in the file Messed up metadata reading from multi-container tables
3 participants