Skip to content
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

Add option to load pointing information with TableLoader, fixes #1900 #1901

Closed
wants to merge 8 commits into from

Conversation

Hckjs
Copy link
Contributor

@Hckjs Hckjs commented Apr 27, 2022

Description:
I've added an option for the TableLoader to load pointing information from the subarray table, interpolate and adding them to the table.

Side-Notes:

  • For now it just adds the (interpolated) /subarray/pointing table instead of the /telescope/pointing as its done for the trigger information. It doesnt make any difference for simulation data or just one telescope for now (but its faster this way), but has to be adapted in future (Therefore the TODO note).
  • I dont know whether its important to get always sorted tables by the keys, but for some parameter combinations, they are not sorted (e.g. if just load_trigger=True, because the astropy_helper function join_allow_empty just copies the unsorted table if one side is empty. The astropy join function sorts by keys). But i think it wouldn't be a problem to just group_by(keys) them, if someone needs a sorted one.

Refers to #1900

@Hckjs Hckjs requested review from maxnoe and kosack April 27, 2022 10:28
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #1901 (2505e88) into master (21c1c3a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1901      +/-   ##
==========================================
+ Coverage   92.05%   92.06%   +0.01%     
==========================================
  Files         189      189              
  Lines       14862    14882      +20     
==========================================
+ Hits        13681    13701      +20     
  Misses       1181     1181              
Impacted Files Coverage Δ
ctapipe/io/tableloader.py 95.65% <100.00%> (+0.41%) ⬆️
ctapipe/io/tests/test_table_loader.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21c1c3a...2505e88. Read the comment docs.

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend not putting the interpolation logic in TableLoader, since it has to be used in other places as well and if we do it differently in two places, it will cause a headache in the future. Better to have a PointingInterpolator component (or even just a function) in the calib module. It should just take an astropy Table as input.

Also, in that you should use scipy.interpolate.interp1d instead of np.interp, since it is more flexible and can pre-compute the interpolation nodes so it is more efficient if one calls it more than once (as we would in the event loop, but maybe not in TableLoader)

@kosack
Copy link
Contributor

kosack commented Apr 27, 2022

see #1902

@maxnoe
Copy link
Member

maxnoe commented Apr 27, 2022

Hi, unfortunately I think it is not as simple as this and I think we have to distinguish the simulation vs. the observed data case here because:

The times in simulations are the timestamp the simulation was performed and not a meaningful timestamps.

This has two consequences:

  • There is no physical relationship between ICRS and AltAz, the simulations are essentially only valid in the local frame
  • Running multiple simulation runs in parallel can theory (and maybe in practice) result in multiple runs at the same time.

I think we should stop storing pointing information like we do it now for simulations and instead use a specialised field / table in simulation/service for that and add a PointingSource interface that also allows plugins (i.e. for reading LST commissioning pointing reports).

@maxnoe
Copy link
Member

maxnoe commented Jul 4, 2022

Closing this for the reasons mentioned above

@maxnoe maxnoe closed this Jul 4, 2022
@Hckjs Hckjs deleted the table_loader_pointings branch February 27, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants