-
Notifications
You must be signed in to change notification settings - Fork 267
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
Container Improvements and Cleanup for DL1 #1301
Conversation
- pulse_time → peak_time (in anticipation of later adding other pulse times, like rise_time, etc.) - added selected_gain_channel (also to DL0, where it was missing)
…nto refactor/containers
EventType was a string, but that doesn't write easily to a file, so now it is an integer Enum
Whole heartedly agree |
- by default not used - if speficied, will fail in Field.validate() - added a Field.validate() and Container.validate() method - fixed a warning in DeprecateField test
I've now gone a bit further and fixed #1224 by adding Field and Container validation. You can now also optionally specify For now, I will start to fill in at least the dtypes and ndims for the DL1 info as needed, rather than relying on the documentation only to specify that. |
…nto refactor/containers
ctapipe/core/container.py
Outdated
@@ -18,33 +24,111 @@ class Field: | |||
Help text associated with the item | |||
unit: `astropy.units.Quantity` | |||
unit to convert to when writing output, or None for no conversion | |||
ucd: str | |||
ucd: str or astropy.unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ucd should not be a unit, right?
Codecov Report
@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 90.49% 90.53% +0.03%
==========================================
Files 184 184
Lines 11883 11980 +97
==========================================
+ Hits 10754 10846 +92
- Misses 1129 1134 +5
Continue to review full report at Codecov.
|
This is just a small first pass of cleaning up the containers relating to DL1. I don't intend this to be the full refactoring of all containers yet, I'm just doing it in small stages. So far, this just removes the need for defining new Containers to make a DL1 dumper (see #1066). For future proposed changes see #1165 and #1224
Container Changes:
DL1CameraContainer
:pulse_time
→peak_time
in anticipation of other pulse times (likerise_time
, etc)selected_gain_channel
(also to DL0 container), since this is neededparameters
as a sub-containerimage_mask
to store the cleaning mask (still need to think what to do in case of non-mask based denoising, but that can come later)MCDL1CameraContainer
that extends this container to add:true_image
(photo electrons from MC without noise)true_parameters
(set of parameters computed from the true_image)DL1CameraContainer
with the prefix set to "true_", though that contains some fields that are maybe not available in the MCsEventIndexContainer
:Features
Fields
now have optionaldtype
,ndim
, andallow_none
metadata attributesField
andContainer
now have.validate()
methods that check the values against the given metadata, if specifiedthings that are not so nice still:
MCDL1CameraContainer
andMCCameraEventContainer
should be merged / cleaned upMCEventContainer
is shower info (could renameMCShowerContainer
instead)MCCameraEventContainer
is not really event-wise info: it contains pointing info (that could be removed), and calibration coefficients that do not change per event. For this reason I do not want to write this container to DL1, since it adds a lot of "images" that do not change. I think the calibration info should move either to be under the the MCHeaderContainer (assuming it does not change during an observation file), or otherwise it could be migrated to the real Calibration containers.There are lots of other general issues, but those can be discussed in the issues linked above.
for reference, here is the MCEventContainer and MCCameraEventContainer(in the latter, I think nearly everything could be moved elsewhere, and the former renamed to "SimulatedShowerContainer")