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

Add write_table and use it in SubarrayDescription.to_hdf, fixes #1449 #1817

Merged
merged 11 commits into from
Mar 29, 2022

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jan 3, 2022

  • This adds a pytables based function to write an astropy table
    into a hdf5 file using the ctapipe conventions.
    This enables writing tables in the same format as used by the
    HDF5TableWriter and those tables can be read using read_table.

  • It also switches SubarrayDescription.to_hdf / from_hdf to use the new
    functions, eliminating the mix of astropy table io using h5py and
    ctapipe hdf5 table io using pytables.

  • ctapipe files should now only contain datasets written by tables and
    use the same way to store metadata consistently.

  • By checking for the existence of __table_column_meta__ datasets, we can stay backwards compatible and still read older subarray files.

  • This now also adds support for reading and writing unicode strings with a max length in utf8 bytes to the hdf io classes and functions.

@maxnoe maxnoe requested a review from kosack January 3, 2022 14:50
@maxnoe maxnoe changed the title Add write_table and use it in SubarrayDescription.to_hdf Add write_table and use it in SubarrayDescription.to_hdf, fixes #1449 Jan 3, 2022
@maxnoe maxnoe linked an issue Jan 3, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #1817 (3acc1cf) into master (d7f8a58) will increase coverage by 0.09%.
The diff coverage is 99.60%.

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
+ Coverage   92.22%   92.31%   +0.09%     
==========================================
  Files         190      191       +1     
  Lines       15196    15388     +192     
==========================================
+ Hits        14015    14206     +191     
- Misses       1181     1182       +1     
Impacted Files Coverage Δ
ctapipe/instrument/subarray.py 93.01% <93.33%> (-0.27%) ⬇️
ctapipe/core/container.py 89.74% <100.00%> (+0.06%) ⬆️
ctapipe/io/__init__.py 100.00% <100.00%> (ø)
ctapipe/io/astropy_helpers.py 93.91% <100.00%> (+4.05%) ⬆️
ctapipe/io/hdf5tableio.py 97.09% <100.00%> (+0.12%) ⬆️
ctapipe/io/tableio.py 93.39% <100.00%> (+0.76%) ⬆️
ctapipe/io/tests/test_astropy_helpers.py 100.00% <100.00%> (ø)
ctapipe/io/tests/test_column_transforms.py 100.00% <100.00%> (ø)
ctapipe/io/tests/test_hdf5.py 99.61% <100.00%> (+0.02%) ⬆️
ctapipe/io/tests/test_write_table.py 100.00% <100.00%> (ø)
... and 1 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 d7f8a58...3acc1cf. Read the comment docs.

@maxnoe maxnoe linked an issue Jan 4, 2022 that may be closed by this pull request
kosack
kosack previously approved these changes Jan 10, 2022
kosack
kosack previously approved these changes Jan 12, 2022
ctapipe/io/astropy_helpers.py Outdated Show resolved Hide resolved
ctapipe/io/astropy_helpers.py Outdated Show resolved Hide resolved
ctapipe/io/hdf5tableio.py Outdated Show resolved Hide resolved
@LukasNickel
Copy link
Member

There are some codacy warning mostly regarding the unit tests. Are you fine with that?
Also the docs are failing, but I dont see the error in the logs

@maxnoe
Copy link
Member Author

maxnoe commented Feb 17, 2022

Rebased vs. master, let's see

kosack
kosack previously approved these changes Mar 18, 2022
Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Looks fine, just need to clean up some unused imports since now those are mandatory. (related - should we remove Codacy from the list of checks since pyflakes is now run? It's nice for history, but always gives lots of errors)

@maxnoe
Copy link
Member Author

maxnoe commented Mar 18, 2022

(related - should we remove Codacy from the list of checks since pyflakes is now run? It's nice for history, but always gives lots of errors)

Pyflakes only checks syntax errors and unused imports, not style. Codacy also checks style.

@kosack
Copy link
Contributor

kosack commented Mar 18, 2022

Pyflakes only checks syntax errors and unused imports, not style. Codacy also checks style.

could add a call to pycodestyle, but yes maybe it's fine now (it's just that we always get a red light from Codacy)

maxnoe added 5 commits March 18, 2022 16:41
* This adds a pytables based function to write an astropy table
into a hdf5 file using the ctapipe conventions.
This enables writing tables in the same format as used by the
HDF5TableWriter and those tables can be read using read_table.

* It also switches SubarrayDescription.to_hdf / from_hdf to use the new
functions, eliminating the mix of astropy table io using h5py and
ctapipe hdf5 table io using pytables.

* ctapipe files should now only contain datasets written by tables and
use the same way to store metadata consistently.
@maxnoe maxnoe requested a review from kosack March 18, 2022 15:42
@maxnoe
Copy link
Member Author

maxnoe commented Mar 25, 2022

@kosack @LukasNickel can you re-approve?

@maxnoe maxnoe merged commit 2923760 into master Mar 29, 2022
@maxnoe maxnoe deleted the write_table branch March 29, 2022 15:04
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.

Do not rely on Table serialize_meta for hdf5 files writing string with HDF5TableWriter
3 participants