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

Inconsistent LSTCam pixels ordering when importing using CameraGeometry.from_name #2484

Closed
abm017 opened this issue Dec 19, 2023 · 5 comments · Fixed by #2485
Closed

Inconsistent LSTCam pixels ordering when importing using CameraGeometry.from_name #2484

abm017 opened this issue Dec 19, 2023 · 5 comments · Fixed by #2485
Labels

Comments

@abm017
Copy link

abm017 commented Dec 19, 2023

The LSTCam pixel positions ordering obtained using CameraGeometry.from_name('LSTCam') differs from the ordering obtained using subarray.tel[1].camera.geometry.pix_x

Consequently, using CameraGeometry.from_name('LSTCam') leads to incorrect LST images as the pixels are arranged differently in the camera plane.

To Reproduce
Steps to reproduce the behavior:
0. ctapipe.__version__ : 0.19.3

  1. Compare CameraGeometry.from_name('LSTCam').pix_x and tel.camera.geometry.pix_x
    (output of latter can also be seen in this tutorial -https://ctapipe.readthedocs.io/en/latest/auto_examples/core/InstrumentDescription.html#explore-some-of-the-details-of-the-telescopes)

Expected behavior
The ordering should be same in both cases (it is same for other telescopes (FlashCam, NectarCam, CHEC)).

Supporting information
Simple plots of pix_x and pix_y positions from both methods.

image
image

Additional context

@abm017 abm017 added the bug label Dec 19, 2023
@kosack
Copy link
Contributor

kosack commented Dec 19, 2023

This is not a bug, but expected behavior. Constructing a CameraGeometry using CameraGeometry.from_name() loads a geometry that was uploaded to the test server for unit tests, and shouldn't be relied upon for general analyses, as we cannot know if it corresponds to the data you are working with. The testing geometries are currently generated from Prod3b simulations, so may not be the same pixel ordering for other simulation productions or for real data. In fact, it's almost certain that the simulated pixel numbering is not the same as the physical cameras.

There are two solutions:

  1. do not use from_name() for data processing, but instead rely on the geometry obtained from the EventSource (for any supported data file) or TableLoader (for ctapipe HDF5 files only), which if implemented correctly for the given input file format, should provide the correct CameraGeometry that corresponds to the data that are loaded.

  2. set the environment variable CTAPIPE_SVC_PATH to include a directory containing the correct geometry FITS files (*.camgeom.fits.gz as generated by e.g. ctapipe-dump-instrument), which will then be the ones loaded by from_name() instead of the defaults from the test server.

Solution 1 is preferred, as it should work in all situations and doesn't rely on the user knowing which geometry corresponds to the data being processed.

@abm017
Copy link
Author

abm017 commented Dec 19, 2023

Ah ok, I did not know of the origin of the geometry that comes from using from_name(). It was just surprising that this only affected LSTCam and none of the other cameras.
No bug/issue in that case.
Maybe Solution 1 can be included in the documentation so that this pitfall can be avoided in general?
(https://ctapipe.readthedocs.io/en/latest/api/ctapipe.instrument.CameraGeometry.html#ctapipe.instrument.CameraGeometry.from_name)
:)

kosack added a commit that referenced this issue Dec 19, 2023
Note that from_name() is not guaranteed to correspond with event data.

Closes #2484
@kosack
Copy link
Contributor

kosack commented Dec 19, 2023

I added some info to the docstring in #2485 to be more clear, since this is not obvious, I agree!

@kosack
Copy link
Contributor

kosack commented Dec 19, 2023

I guess once we have stable hardware and simulations, we could ensure that the loaded instrument info is the same, but for now, we keep changing things so that won't work! Originally I had envisioned the ability to switch between different sets of test data as in #738, but so far that's not implemented. In any case, it's always best to rely on reading the data from the exact file you are using. For observed, DL0 data products which currently do not contain this info, we will have to develop a solution.

@kosack
Copy link
Contributor

kosack commented Dec 19, 2023

We may also just remove these methods now that we have real and simulated data to work with (#1978)

kosack added a commit that referenced this issue Jan 24, 2024
Note that from_name() is not guaranteed to correspond with event data.

Closes #2484
maxnoe pushed a commit that referenced this issue Mar 22, 2024
Note that from_name() is not guaranteed to correspond with event data.

Closes #2484
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants