Store both equivalent and effective focal length in OpticsDescription, add __slots__ #1923
Conversation
maxnoe
force-pushed
the
optics_effective_focal_length
branch
3 times, most recently
from
0739a81 to
b31fdec
Compare
Codecov Report
@@ Coverage Diff @@
## master #1923 +/- ##
==========================================
- Coverage 92.35% 92.34% -0.01%
==========================================
Files 193 193
Lines 15585 15621 +36
==========================================
+ Hits 14393 14425 +32
- Misses 1192 1196 +4
Continue to review full report at Codecov.
|
maxnoe
force-pushed
the
optics_effective_focal_length
branch
from
b31fdec to
5ed177c
Compare
|
Let's merge this PR and the other one affecting the data model: #1949 and then I can recreate the higher level test data files with the new data model for the machine learning module. |
maxnoe
force-pushed
the
optics_effective_focal_length
branch
from
f31afde to
91f9510
Compare
maxnoe
force-pushed
the
optics_effective_focal_length
branch
from
91f9510 to
19bdf73
Compare
maxnoe
force-pushed
the
optics_effective_focal_length
branch
2 times, most recently
from
b92929b to
41abc58
Compare
maxnoe
force-pushed
the
optics_effective_focal_length
branch
from
41abc58 to
687f38c
Compare
ctapipe/instrument/subarray.py
Outdated
| @@ -538,7 +547,7 @@ def to_hdf(self, h5file, overwrite=False, mode="a"): | |||
| meta["reference_itrs_z"] = itrs.z.to_value(u.m) | |||
|
|
|||
| @classmethod | |||
| def from_hdf(cls, path): | |||
| def from_hdf(cls, path, focal_length_choice="effective"): | |||
There was a problem hiding this comment.
do we expect other possibilities than equivalent and effective in the future?
If not, a boolean effective=True might be less error prone than typing a string?
There was a problem hiding this comment.
Good point, I will use an enum instead
There was a problem hiding this comment.
it's possible that in the future this is a calibration parameter that is measured in-situ as well. I know in HESS, for example, we store both the nominal and measured and have to correct for the difference, and for the HESS CT5 telescope, there is even an autofocus system that can dynamically change the focal length. So I agree that an ENUM is best here, in case we add other ways to obtain this information.
Only for single mirror telescopes, this is why the docstring says: "This is the nominal focal length for single mirror telescopes and the equivalent focal length for dual mirror telescopes". For dual mirror telescopes, there are two nominal focal lengths and the equivalent focal length is the thin lens focal length that would give the same image. |
Sorry I read too quickly. Thank you for the explanation. |
|
Sorry @kosack, had to fix the docs build |
This is adding a new field
effective_focal_lengthto the optics description, so we don't need to choose there but when we actually do the transforms.Like this, it's clear inside ctapipe what focal length is used when and it doesn't depend on options what is inside the OpticsDescription.
The choice options now say which of the focal lengths is attached to the
CameraGeometry.frameattribute when creating the subarray, so all things loading a subarray (SimTelEventSource, HDF5EventSource, Subarray.from_hdf, TableLoader) now gain this option.