-
Notifications
You must be signed in to change notification settings - Fork 269
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
Change R1- and DL0-waveforms shape to (n_channels, n_pixels, n_samples) #2529
Conversation
This comment has been minimized.
This comment has been minimized.
I tend agree. At least for now we can leave this unsupported. Just make sure there is a nice error message in case someone tries to apply it to two-gain data |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2529 +/- ##
==========================================
+ Coverage 92.66% 92.67% +0.01%
==========================================
Files 232 231 -1
Lines 20220 20292 +72
==========================================
+ Hits 18736 18805 +69
- Misses 1484 1487 +3 ☔ View full report in Codecov by Sentry. |
I guess with this we break the reading-in of R0, R1 and DL0 data written into HDF5 files? I.e. this is a data model change that requires changes in the compatibility table of the |
This comment has been minimized.
This comment has been minimized.
At the moment (lst-cluster down), I can only test it with the LST-1 HDF5 files of R1 calibration data, which comes with two gains. This is working fine for me with the
|
That case will work fine, the issue will be reading single-gain data written with older versions which will result in the 2d shape that is written into the file not the 3d shape expected now |
I don't understand the relevance of DL0 ACADA (zfits) data in the discussion about the HDF5 event source... |
Yes, sorry! This is what I meant. I accidentally thought that this applies to DL0 ACADA data. However, as you said it's stored (of course) in zfits and not HDF5! |
Is there a good reason to care about backward compatibility here? I mean, do you want a workaround to be implemented to remain compatible with files written by previous versions or just update the Edit: if we don't care about backwards compatibility, one line change here shall be enough, right? |
I have tested it with an old LST-1 sim file in hdf5, where the R1 waveform data is stored with 2 dims, i.e. shape of waveform: (1855,40). In case you would like to have backward compatibility, I did the following:
Since the
I'm having some trouble with |
I don't have a strong opinion here, but we need to do one of two things:
|
Due to some version incompatibilities, the array pointing information was not written to the HDF5 file. When reading the incomplete/corrupted file the |
I'm in favor of raising a nice error. I don't think there are many (and crucial) R1 or DL0 datasets produced in HDF5 following the 2d waveform shape. Most users are only storing DL1b and/or DL1a. (I actually have a R1 production of LST-1 sims for DL studies. However, it can be easily reprocessed with this PR!) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The docs failure is due to an unclosed tables file, I think the one that is opened on line 222 of Either you close the file, or you add an exception for that warning into the docs conf |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Adapted extractors and calibrator to handel broken pixels for multiple gain channels - InvalidPixelHandel cannot handle multiple gain channels for now
- I actually cant remember why i used transpose to broadcast the calib coeffs to the charges and peak times. I think it was due to a usecase which is actually not needed. - I've added a test especially for 2 gain data to check that the calibrator also works this way
Co-Authored-By: Jonas Hackfeld <[email protected]>
- _apply_correction is now a static method from ImageExtractors parent class - _calculate_correction is moved to the parent ImageExtractors class to avoid writing the docstrings multiple times. It needs to be overwritten by the child components
- it duplicated the changes made in main, had to remove one of them
This comment has been minimized.
This comment has been minimized.
1 similar comment
Analysis Details7 IssuesCoverage and DuplicationsProject ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs |
This PR basically adds three (somehow dependent) features:
Changing the R1 and DL0
waveforms
shape to be always(n_channels, n_pixels, n_samples)
.Implementing 1) in such a general way (and two lines of code) allows the
ImageExtractors
to handle not gain selected data (by passingselected_gain_channel=None
)To test these i've adapted the
WaveformToymodel
to be able to create 2 gain waveforms. All extractors are now tested for different camera types (including different number of gain channels)Things that probably still need to be discussed:
DL1CameraContainer
considering the two different ndims ofimage
andpeak_time
GainSelector
it cannot distinguish between already gain selected data and data that originally consist of just one gain channel. We may pass something likecamera.readout.n_channels
?Closes #1836
Thanks already to @TjarkMiener for the discussion and input!