Repository: cta-observatory/ctapipe (python)

Issue:
Inconsistent LSTCam pixels ordering when importing using CameraGeometry.from_name
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](https://github.com/cta-observatory/ctapipe/assets/56478499/e81bcbde-c809-4c49-a3a4-ae7ebb8a1b0e)
![image](https://github.com/cta-observatory/ctapipe/assets/56478499/ccfc8d0b-4f37-4c8a-99be-fb3a11906dd9)

**Additional context**



Pull request description:
Add a note to CameraGeometry docs about from_name()
Note that from_name() is not guaranteed to correspond with event data.

Closes #2484

Patch:
diff --git a/docs/changes/2485.maintenance.rst b/docs/changes/2485.maintenance.rst
new file mode 100644
index 00000000..52f136d1
--- /dev/null
+++ b/docs/changes/2485.maintenance.rst
@@ -0,0 +1,7 @@
+The ``from_name`` methods of instrument description classes now raise a warning
+that it is better to access instrument information via a ``SubarrayDescription``.
+
+Also improve documentation in instrument module to explain when not to use the
+various ``from_name()`` methods. These are provided for the case when no event
+data is available, e.g. for unit testing or demos, but do not guarantee that the
+returned instrument information corresponds to a particular set of event data.
diff --git a/pyproject.toml b/pyproject.toml
index efcf0fd5..fd9417b8 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -141,6 +141,7 @@ addopts = ["-ra", "--strict-config", "--strict-markers"]
 filterwarnings = [
     "error::astropy.utils.exceptions.AstropyDeprecationWarning",
     "error::ctapipe.utils.deprecation.CTAPipeDeprecationWarning",
+    "error::ctapipe.instrument.FromNameWarning",
     "ignore:`np.MachAr` is deprecated:DeprecationWarning",
 ]
 norecursedirs = [
diff --git a/src/ctapipe/instrument/__init__.py b/src/ctapipe/instrument/__init__.py
index 16c96403..b7c540d3 100644
--- a/src/ctapipe/instrument/__init__.py
+++ b/src/ctapipe/instrument/__init__.py
@@ -5,6 +5,7 @@ from .optics import FocalLengthKind, OpticsDescription, ReflectorShape, SizeType
 from .subarray import SubarrayDescription, UnknownTelescopeID
 from .telescope import TelescopeDescription
 from .trigger import SoftwareTrigger
+from .warnings import FromNameWarning
 
 __all__ = [
     "CameraDescription",
@@ -21,4 +22,5 @@ __all__ = [
     "ReflectorShape",
     "SizeType",
     "SoftwareTrigger",
+    "FromNameWarning",
 ]
diff --git a/src/ctapipe/instrument/camera/description.py b/src/ctapipe/instrument/camera/description.py
index b88bc1fa..99eb28a9 100644
--- a/src/ctapipe/instrument/camera/description.py
+++ b/src/ctapipe/instrument/camera/description.py
@@ -4,6 +4,7 @@ Classes pertaining to the description of a Cherenkov camera
 
 from ctapipe.utils import find_all_matching_datasets
 
+from ..warnings import warn_from_name
 from .geometry import CameraGeometry
 from .readout import CameraReadout
 
@@ -60,19 +61,32 @@ class CameraDescription:
 
     @classmethod
     def from_name(cls, name):
-        """
-        Construct a CameraDescription from a camera name
+        """Construct a CameraDescription from a camera name
 
         Parameters
         ----------
         name: str
             Camera name (e.g. NectarCam, LSTCam, ...)
 
+
+        Notes
+        -----
+
+        Warning: This method loads a pre-generated ``CameraDescription`` and is
+        thus not guaranteed to be the same pixel ordering or even positions that
+        correspond with event data! Therefore if you are analysing data, you
+        should not rely on this method, but rather open the data with an
+        ``EventSource`` and use the ``CameraDescription`` that is provided by
+        ``source.subarray.tel[i].camera`` or by
+        ``source.subarray.camera_types[type_name]``. This will guarantee that
+        the pixels in the event data correspond with the ``CameraDescription``
+
         Returns
         -------
         CameraDescription
 
         """
+        warn_from_name()
 
         geometry = CameraGeometry.from_name(name)
         try:
diff --git a/src/ctapipe/instrument/camera/geometry.py b/src/ctapipe/instrument/camera/geometry.py
index 92dd12f9..66612bdd 100644
--- a/src/ctapipe/instrument/camera/geometry.py
+++ b/src/ctapipe/instrument/camera/geometry.py
@@ -19,6 +19,7 @@ from ctapipe.coordinates import CameraFrame, get_representation_component_names
 from ctapipe.utils import get_table_dataset
 from ctapipe.utils.linalg import rotation_matrix_2d
 
+from ..warnings import warn_from_name
 from .image_conversion import (
     get_orthogonal_grid_edges,
     get_orthogonal_grid_indices,
@@ -581,13 +582,25 @@ class CameraGeometry:
 
     @classmethod
     def from_name(cls, name="NectarCam", version=None):
-        """
-        Construct a CameraGeometry using the name of the camera and array.
+        """Construct a CameraGeometry using the name of the camera and array.
 
         This expects that there is a resource accessible via
         `~ctapipe.utils.get_table_dataset` called ``"[array]-[camera].camgeom.fits.gz"``
         or ``"[array]-[camera]-[version].camgeom.fits.gz"``
 
+        Notes
+        -----
+
+        Warning: This method loads a pre-generated ``CameraGeometry`` and is
+        thus not guaranteed to be the same pixel ordering or even positions that
+        correspond with event data! Therefore if you are analysing data, you
+        should not rely on this method, but rather open the data with an
+        ``EventSource`` and use the ``CameraGeometry`` that is provided by
+        ``source.subarray.tel[i].camera.geometry`` or by
+        ``source.subarray.camera_types[type_name].geometry``. This will
+        guarantee that the pixels in the event data correspond with the
+        ``CameraGeometry``
+
         Parameters
         ----------
         name : str
@@ -598,7 +611,9 @@ class CameraGeometry:
         Returns
         -------
         new CameraGeometry
+
         """
+        warn_from_name()
 
         if version is None:
             verstr = ""
diff --git a/src/ctapipe/instrument/camera/readout.py b/src/ctapipe/instrument/camera/readout.py
index 73e8c1d1..3a886942 100644
--- a/src/ctapipe/instrument/camera/readout.py
+++ b/src/ctapipe/instrument/camera/readout.py
@@ -10,6 +10,8 @@ from astropy.table import Table
 
 from ctapipe.utils import get_table_dataset
 
+from ..warnings import warn_from_name
+
 __all__ = ["CameraReadout"]
 
 logger = logging.getLogger(__name__)
@@ -144,8 +146,8 @@ class CameraReadout:
         Returns
         -------
         new CameraReadout
-
         """
+        warn_from_name()
 
         if version is None:
             verstr = ""
diff --git a/src/ctapipe/instrument/optics.py b/src/ctapipe/instrument/optics.py
index b6c4d3a7..ddec040f 100644
--- a/src/ctapipe/instrument/optics.py
+++ b/src/ctapipe/instrument/optics.py
@@ -11,6 +11,7 @@ from astropy.table import QTable
 
 from ..compat import StrEnum
 from ..utils import get_table_dataset
+from .warnings import warn_from_name
 
 logger = logging.getLogger(__name__)
 
@@ -195,6 +196,8 @@ class OpticsDescription:
         OpticsDescription
 
         """
+        warn_from_name()
+
         if isinstance(optics_table, str):
             table = get_table_dataset(optics_table, role="OpticsDescription.from_name")
         else:
diff --git a/src/ctapipe/instrument/telescope.py b/src/ctapipe/instrument/telescope.py
index edab4e24..b0a735b9 100644
--- a/src/ctapipe/instrument/telescope.py
+++ b/src/ctapipe/instrument/telescope.py
@@ -17,6 +17,7 @@ from ..coordinates import CameraFrame
 from .camera import CameraDescription
 from .guess import guess_telescope, unknown_telescope
 from .optics import OpticsDescription
+from .warnings import warn_from_name
 
 __all__ = ["TelescopeDescription"]
 
@@ -84,11 +85,25 @@ class TelescopeDescription:
            optics name (e.g. LST, or SST-ASTRI), also called
            telescope_description
 
+        Notes
+        -----
+
+        Warning: This method loads a pre-generated ``TelescopeDescription`` and is
+        thus not guaranteed to be the same pixel ordering or even positions that
+        correspond with event data! Therefore if you are analysing data, you
+        should not rely on this method, but rather open the data with an
+        ``EventSource`` and use the ``TelescopeDescription`` that is provided by
+        ``source.subarray.tel[i]`` or by
+        ``source.subarray.telescope_types[type_name]``. This will guarantee that
+        the pixels in the event data correspond with the ``TelescopeDescription``
+
+
         Returns
         -------
         TelescopeDescription
 
         """
+        warn_from_name()
 
         camera = CameraDescription.from_name(camera_name)
         optics = OpticsDescription.from_name(optics_name)
diff --git a/src/ctapipe/instrument/warnings.py b/src/ctapipe/instrument/warnings.py
new file mode 100644
index 00000000..1344cbab
--- /dev/null
+++ b/src/ctapipe/instrument/warnings.py
@@ -0,0 +1,18 @@
+"""Warnings related to the instrument"""
+import warnings
+
+__all__ = ["FromNameWarning", "warn_from_name"]
+
+
+class FromNameWarning(UserWarning):
+    """Warning raised when using from_name"""
+
+
+def warn_from_name():
+    """Warning raised when accessing .from_name methods"""
+    msg = (
+        ".from_name uses pre-defined data that is"
+        " likely different from the data being analyzed."
+        " Access instrument information via the SubarrayDescription instead."
+    )
+    warnings.warn(msg, FromNameWarning, stacklevel=2)


Test patch:
diff --git a/src/ctapipe/conftest.py b/src/ctapipe/conftest.py
index 584385ec..42d53fc0 100644
--- a/src/ctapipe/conftest.py
+++ b/src/ctapipe/conftest.py
@@ -10,7 +10,7 @@ from astropy.coordinates import EarthLocation
 from pytest_astropy_header.display import PYTEST_HEADER_MODULES
 
 from ctapipe.core import run_tool
-from ctapipe.instrument import CameraGeometry, SubarrayDescription
+from ctapipe.instrument import CameraGeometry, FromNameWarning, SubarrayDescription
 from ctapipe.io import SimTelEventSource
 from ctapipe.utils import get_dataset_path
 from ctapipe.utils.datasets import resource_file
@@ -43,7 +43,8 @@ camera_names = [
 
 @pytest.fixture(scope="function", params=camera_names)
 def camera_geometry(request):
-    return CameraGeometry.from_name(request.param)
+    with pytest.warns(FromNameWarning):
+        return CameraGeometry.from_name(request.param)
 
 
 @pytest.fixture(scope="session")
diff --git a/src/ctapipe/image/tests/test_cleaning.py b/src/ctapipe/image/tests/test_cleaning.py
index c5694f58..1e799a4d 100644
--- a/src/ctapipe/image/tests/test_cleaning.py
+++ b/src/ctapipe/image/tests/test_cleaning.py
@@ -334,8 +334,8 @@ def test_apply_time_delta_cleaning(prod3_lst):
     assert (test_mask == td_mask).all()
 
 
-def test_time_constrained_clean():
-    geom = CameraGeometry.from_name("LSTCam")
+def test_time_constrained_clean(prod5_lst):
+    geom = prod5_lst.camera.geometry
     charge = np.zeros(geom.n_pixels, dtype=np.float64)
     peak_time = np.zeros(geom.n_pixels, dtype=np.float64)
 
diff --git a/src/ctapipe/instrument/camera/tests/test_geometry.py b/src/ctapipe/instrument/camera/tests/test_geometry.py
index a18576e1..df90a1ba 100644
--- a/src/ctapipe/instrument/camera/tests/test_geometry.py
+++ b/src/ctapipe/instrument/camera/tests/test_geometry.py
@@ -7,6 +7,7 @@ from astropy import units as u
 from astropy.coordinates import AltAz, SkyCoord
 
 from ctapipe.instrument import CameraGeometry, PixelShape
+from ctapipe.instrument.warnings import FromNameWarning
 
 
 def test_construct():
@@ -306,7 +307,8 @@ def test_hashing(prod5_lst, prod5_mst_nectarcam):
 
 def test_camera_from_name(camera_geometry):
     """check we can construct all cameras from name"""
-    camera = CameraGeometry.from_name(camera_geometry.name)
+    with pytest.warns(FromNameWarning):
+        camera = CameraGeometry.from_name(camera_geometry.name)
     assert str(camera) == camera_geometry.name
 
 
diff --git a/src/ctapipe/instrument/camera/tests/test_readout.py b/src/ctapipe/instrument/camera/tests/test_readout.py
index 509451fb..c2debb6f 100644
--- a/src/ctapipe/instrument/camera/tests/test_readout.py
+++ b/src/ctapipe/instrument/camera/tests/test_readout.py
@@ -5,7 +5,7 @@ import numpy as np
 import pytest
 from astropy import units as u
 
-from ctapipe.instrument import CameraReadout
+from ctapipe.instrument import CameraReadout, FromNameWarning
 
 
 def test_construct():
@@ -186,7 +186,8 @@ def test_hashing():
 @pytest.mark.parametrize("name", ["LSTCam", "FlashCam", "NectarCam", "CHEC"])
 def test_camera_from_name(name, svc_path):
     """check we can construct all cameras from name"""
-    camera = CameraReadout.from_name(name)
+    with pytest.warns(FromNameWarning):
+        camera = CameraReadout.from_name(name)
     assert str(camera) == name
 
 
diff --git a/src/ctapipe/instrument/tests/test_optics.py b/src/ctapipe/instrument/tests/test_optics.py
index 11ce4df4..d40b6568 100644
--- a/src/ctapipe/instrument/tests/test_optics.py
+++ b/src/ctapipe/instrument/tests/test_optics.py
@@ -4,6 +4,7 @@ import pytest
 from astropy import units as u
 
 from ctapipe.instrument.optics import OpticsDescription, ReflectorShape, SizeType
+from ctapipe.instrument.warnings import FromNameWarning
 
 
 def test_guess_optics():
@@ -50,6 +51,7 @@ def test_construct_optics():
 )
 def test_optics_from_name(optics_name, focal_length, svc_path):
     # test with file written by dump-instrument
-    optics = OpticsDescription.from_name(optics_name)
+    with pytest.warns(FromNameWarning):
+        optics = OpticsDescription.from_name(optics_name)
     assert optics.name == optics_name
     assert u.isclose(optics.equivalent_focal_length, focal_length)
diff --git a/src/ctapipe/instrument/tests/test_telescope.py b/src/ctapipe/instrument/tests/test_telescope.py
index 4adfa73e..901f9a99 100644
--- a/src/ctapipe/instrument/tests/test_telescope.py
+++ b/src/ctapipe/instrument/tests/test_telescope.py
@@ -1,6 +1,7 @@
 """ Tests for TelescopeDescriptions """
 import pytest
 
+from ctapipe.instrument import FromNameWarning
 from ctapipe.instrument.telescope import TelescopeDescription
 
 
@@ -36,7 +37,8 @@ def test_hash(subarray_prod5_paranal):
 @pytest.mark.parametrize("optics_name", ["LST", "MST", "ASTRI"])
 def test_telescope_from_name(optics_name, camera_name, svc_path):
     """Check we can construct all telescopes from their names"""
-    tel = TelescopeDescription.from_name(optics_name, camera_name)
+    with pytest.warns(FromNameWarning):
+        tel = TelescopeDescription.from_name(optics_name, camera_name)
 
     assert optics_name in str(tel)
     assert camera_name in str(tel)