search

cta-observatory / cta-lstchain

LST prototype testbench chain
https://cta-observatory.github.io/cta-lstchain/
BSD 3-Clause "New" or "Revised" License
22 stars 77 forks source link

Add possibility to copy and not merge some keys by lstchain_merge_hdf5_files #1174

Closed FrancaCassol closed 7 months ago

FrancaCassol commented 8 months ago

This is in particular necessary for the calibration keys in DL1 and DL2 files, which are the same for all subruns and should not be merged but simply copied from the first file of the list. I wonder if we can already add them as default in the script...

vuillaut commented 8 months ago

Hi Should these keys be added to merging_checks to make sure they are indeed the same in all files? (open question)

codecov[bot] commented 8 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e82bd68) 74.39% compared to head (fd7aa94) 74.40%.

Files Patch % Lines
lstchain/io/io.py 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1174 +/- ## ========================================== + Coverage 74.39% 74.40% +0.01% ========================================== Files 129 129 Lines 13195 13206 +11 ========================================== + Hits 9816 9826 +10 - Misses 3379 3380 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maxnoe commented 8 months ago

This special handling of some keys is also needed to fix https://github.com/cta-observatory/cta-lstchain/issues/1152

So we should just make a set of keys that are global and are copied from the first file (potentially checking that they are equal in all following files) and keys that are "event-like" so need to be merged.

FrancaCassol commented 7 months ago

Hi @maxnoe , @morcuended , I added the default keys, but I prefer to keep the trailet in case of tests on new implementations. Do not hesitate to add changes if you have strong opinions on this. We need to merge this PR before next release, which should be done soon.

rlopezcoto commented 7 months ago

@maxnoe , @morcuended are you ok with the changes so we can move on with this PR as well?