-
Notifications
You must be signed in to change notification settings - Fork 272
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
Interpolate pointing in TableLoader and HDF5EventSource #2409
Conversation
Great! this can also be used eventually to fill in the event-wise monitoring info in the event loop as well not just in TableLoader. And just to note, this eventually must also interpolate other mis-pointing parameters like camera-z-axis rotation and any other deformation that needs to be applied during the DL1 step. LST doesn't compute those yet I guess, but it's good not to forget they will be necessary eventually! |
d42d561
to
a872749
Compare
8eb4cf1
to
7fa6a73
Compare
d11ecd3
to
2f16e3b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2409 +/- ##
==========================================
+ Coverage 92.59% 92.64% +0.04%
==========================================
Files 232 234 +2
Lines 20034 20223 +189
==========================================
+ Hits 18551 18736 +185
- Misses 1483 1487 +4 ☔ View full report in Codecov by Sentry. |
2f16e3b
to
f1c90a1
Compare
f45babe
to
7d14fe1
Compare
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.
Looks good to me
ctapipe/io/pointing.py
Outdated
# [359, 1] the telescope moved 2 degrees through 0, not 358 degrees | ||
# the other way around. This should be true for all telescopes given | ||
# the sampling speed of pointing values and their maximum movement speed. | ||
# No telescope can turn more than 180° in 2 seconds. |
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.
I think the more relevant remark is that during tracking of a source the telescopes only move quickly close to the poles, and the worst case for those motions are a straight overhead track that flip the Azimuth 180 degrees. But maybe I misunderstand this comment.
I expect how this works in practice depends on how the telescopes handle the very high zenith tracks, do they stop data taking as they swing around?
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.
I think there is a telescope requirement that we do not observe very close to a poll (though I have to check exactly how far is allowed). So probably it's not a major issue, at least for CTA. For HESS, for example CT5 can in principle move to altitudes greater than 90°, but in practice due to the effects it has on pointing accuracy, it's no longer used (and the fact that the HESS software also doesn't do this transform correctly).
It may be worth adding some of this comment's text to the docstring of the class, however, as it is important to remember and should show up in the documentation.
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.
I think the more relevant remark is that during tracking of a source the telescopes only move quickly close to the poles,
What is implemented and tested here is really the normal case of moving, not related to any movement at the pole, given the sequence of azimuth values and assuming you interpolate exactly in between, there are 2 solutions for the sequence:
[357, 359, 1, 3]
Interpolating naïvely:
[358, 180, 2]
or with the numpy function applied above, the sequence is first turned into:
[357, 359, 361, 363]
which is then interpolated and wrapped to:
[358, 0, 2]
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.
only minor comments (docstring fixes). Otherwise looks ok.
ctapipe/io/pointing.py
Outdated
default_value=False, | ||
).tag(config=True) | ||
|
||
def __init__(self, h5file=None, **kwargs): |
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.
please document the h5file argument (what is expected, what happens if it's None). E.g. if h5file is specified, PointingInterpolator will look for the tables in [path] and load all that are available for each tel_id. If it is not specified, the user should call add_table()
to add tables for relevant tel_ids.
The ``dl0/monitoring/telescope/pointing/tel_{tel_id:03d}`` tables are not yet writte by ctapipe itself, but can be added to files by external tools to enable analysis of real observations.
10927a9
to
9ee37cb
Compare
Analysis Details3 IssuesCoverage and DuplicationsProject ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs |
This adds the possibility to interpolate pointing information from a not yet used by ctapipe itself
/dl0/monitoring/telescope/pointing/<tel_id>
group.This is critical for analysis of e.g. LST observations.
At the moment, I add this table to the DL1 files output by ctapipe-process with a custom script, but eventually we should also add APIs for the datawriter / ctapipe-process / event sources to read this from monitoring files and put it into the outputs.