Repository: cta-observatory/ctapipe
Pull request #2443 (2023-10-31 14:32:24)

Improve column names for disp
- This removes the `disp_parameter_is_valid` column, since it is identical to the `disp_tel_is_valid` column.
- Doing this makes it possible to use the same prefix for both the `DispContainer` and the `ReconstructedGeometryContainer`, as filled by `DispReconstructor`. 
- As the `DispContainer` contains the output values of both the regressor (norm) and the classifier (sign) as a single value (sign * norm), I renamed this value to "parameter" to avoid confusion.

**HOWEVER:** `DispReconstructor` still creates all the geometry-related columns contained in `ReconstructedGeometryContainer` even though it only computes altitude and azimuth. But since the creation of such empty columns is not limited to the disp reconstructor, I think it would be better to address this in a different PR. 

maxnoe: Related: https://github.com/cta-observatory/ctapipe/issues/2228

We could have a `ReconstructedDirectionContainer`, a `ReconstructedImpactContainer` and a `ReconstructedGeometryContainer` that combines the first two for algorithms that do all on one go. 
maxnoe: And some tests and more code has to be adapted, see the test failures.

Patch:
diff --git a/ctapipe/containers.py b/ctapipe/containers.py
index 3fbb88a7..d94f39ee 100644
--- a/ctapipe/containers.py
+++ b/ctapipe/containers.py
@@ -988,10 +988,11 @@ class DispContainer(Container):
     Standard output of disp reconstruction algorithms for origin reconstruction
     """
 
-    default_prefix = "disp_parameter"
+    default_prefix = "disp"
 
-    norm = Field(nan * u.deg, "reconstructed value for disp", unit=u.deg)
-    is_valid = Field(False, "true if the predictions are valid")
+    parameter = Field(
+        nan * u.deg, "reconstructed value for disp (= sign * norm)", unit=u.deg
+    )
 
 
 class ReconstructedContainer(Container):
diff --git a/ctapipe/io/datawriter.py b/ctapipe/io/datawriter.py
index 3a5cc8f0..398e4acb 100644
--- a/ctapipe/io/datawriter.py
+++ b/ctapipe/io/datawriter.py
@@ -48,8 +48,11 @@ def _get_tel_index(event, tel_id):
 #   (meaning readers need to update scripts)
 # - increase the minor number if new columns or datasets are added
 # - increase the patch number if there is a small bugfix to the model.
-DATA_MODEL_VERSION = "v5.0.0"
+DATA_MODEL_VERSION = "v5.1.0"
 DATA_MODEL_CHANGE_HISTORY = """
+- v5.1.0: - Remove redundant 'is_valid' column in ``DispContainer``.
+          - Rename content of ``DispContainer`` from 'norm' to 'parameter' and use the same
+            default prefix ('disp') for all containers filled by ``DispReconstructor``.
 - v5.0.0: - Change DL2 telescope-wise container prefixes from {algorithm}_tel to {algorithm}_tel_{kind}.
             As of now, this only changes 'tel_distance' to 'tel_impact_distance'
 - v4.0.0: - Changed how ctapipe-specific metadata is stored in hdf5 attributes.
@@ -58,8 +61,8 @@ DATA_MODEL_CHANGE_HISTORY = """
             and true parameters.
           - Telescope Impact Parameters were added.
           - Effective focal length and nominal focal length are both included
-            in the optics description now. Moved `TelescopeDescription.type`
-            to `OpticsDescription.size_type`. Added `OpticsDescription.reflector_shape`.
+            in the optics description now. Moved ``TelescopeDescription.type``
+            to ``OpticsDescription.size_type``. Added ``OpticsDescription.reflector_shape``.
           - n_samples, n_samples_long, n_channels and n_pixels are now part
             of CameraReadout.
           - The reference_location (EarthLocation origin of the telescope coordinates)
diff --git a/ctapipe/io/hdf5eventsource.py b/ctapipe/io/hdf5eventsource.py
index c7bda1f9..9b7e533d 100644
--- a/ctapipe/io/hdf5eventsource.py
+++ b/ctapipe/io/hdf5eventsource.py
@@ -16,6 +16,7 @@ from ..containers import (
     CameraHillasParametersContainer,
     CameraTimingParametersContainer,
     ConcentrationContainer,
+    DispContainer,
     DL1CameraContainer,
     EventIndexContainer,
     HillasParametersContainer,
@@ -64,12 +65,14 @@ DL2_CONTAINERS = {
     "geometry": ReconstructedGeometryContainer,
     "classification": ParticleClassificationContainer,
     "impact": TelescopeImpactParameterContainer,
+    "disp": DispContainer,
 }
 
 
 COMPATIBLE_DATA_MODEL_VERSIONS = [
     "v4.0.0",
     "v5.0.0",
+    "v5.1.0",
 ]
 
 
diff --git a/ctapipe/reco/sklearn.py b/ctapipe/reco/sklearn.py
index 1ae0bc78..8d5d7fe9 100644
--- a/ctapipe/reco/sklearn.py
+++ b/ctapipe/reco/sklearn.py
@@ -493,7 +493,7 @@ class DispReconstructor(Reconstructor):
     Predict absolute value and sign for disp origin reconstruction for each telescope.
     """
 
-    target = "true_norm"
+    target = "true_disp"
 
     prefix = traits.Unicode(default_value="disp", allow_none=False).tag(config=True)
 
@@ -690,12 +690,10 @@ class DispReconstructor(Reconstructor):
 
             if passes_quality_checks:
                 disp, valid = self._predict(self.subarray.tel[tel_id], table)
-                disp_container = DispContainer(
-                    norm=disp[0],
-                    is_valid=valid[0],
-                )
 
                 if valid:
+                    disp_container = DispContainer(parameter=disp[0])
+
                     hillas = event.dl1.tel[tel_id].parameters.hillas
                     psi = hillas.psi.to_value(u.rad)
 
@@ -715,6 +713,9 @@ class DispReconstructor(Reconstructor):
                     )
 
                 else:
+                    disp_container = DispContainer(
+                        parameter=u.Quantity(np.nan, self.unit),
+                    )
                     altaz_container = ReconstructedGeometryContainer(
                         alt=u.Quantity(np.nan, u.deg, copy=False),
                         az=u.Quantity(np.nan, u.deg, copy=False),
@@ -722,8 +723,7 @@ class DispReconstructor(Reconstructor):
                     )
             else:
                 disp_container = DispContainer(
-                    norm=u.Quantity(np.nan, self.unit),
-                    is_valid=False,
+                    parameter=u.Quantity(np.nan, self.unit),
                 )
                 altaz_container = ReconstructedGeometryContainer(
                     alt=u.Quantity(np.nan, u.deg, copy=False),
@@ -731,7 +731,7 @@ class DispReconstructor(Reconstructor):
                     is_valid=False,
                 )
 
-            disp_container.prefix = f"{self.prefix}_parameter"
+            disp_container.prefix = f"{self.prefix}_tel"
             altaz_container.prefix = f"{self.prefix}_tel"
             event.dl2.tel[tel_id].disp[self.prefix] = disp_container
             event.dl2.tel[tel_id].geometry[self.prefix] = altaz_container
@@ -763,18 +763,12 @@ class DispReconstructor(Reconstructor):
         valid = self.quality_query.get_table_mask(table)
         disp[valid], is_valid[valid] = self._predict(key, table[valid])
 
-        disp_result = Table(
-            {
-                f"{self.prefix}_parameter_norm": disp,
-                f"{self.prefix}_parameter_is_valid": is_valid,
-            }
-        )
+        disp_result = Table({f"{self.prefix}_tel_parameter": disp})
         add_defaults_and_meta(
             disp_result,
             DispContainer,
-            prefix=f"{self.prefix}_parameter",
-            # disp is always per telescope, so no need to add the prefix
-            add_tel_prefix=False,
+            prefix=self.prefix,
+            add_tel_prefix=True,
         )
 
         psi = table["hillas_psi"].quantity.to_value(u.rad)
diff --git a/docs/changes/2443.datamodel.rst b/docs/changes/2443.datamodel.rst
new file mode 100644
index 00000000..0d978893
--- /dev/null
+++ b/docs/changes/2443.datamodel.rst
@@ -0,0 +1,6 @@
+Remove redundant ``is_valid`` field in ``DispContainer`` and rename the remaining field.
+Use the same prefix for both containers filled by ``DispReconstructor``.
+
+Fix default name of ``DispReconstructor`` target column.
+
+Let ``HDF5EventSource`` load ``DispContainer``.


Test patch:
diff --git a/ctapipe/io/tests/test_hdf5eventsource.py b/ctapipe/io/tests/test_hdf5eventsource.py
index bae1eb13..d64e2ed7 100644
--- a/ctapipe/io/tests/test_hdf5eventsource.py
+++ b/ctapipe/io/tests/test_hdf5eventsource.py
@@ -28,7 +28,7 @@ def test_is_compatible(compatible_file, request):
 def test_metadata(dl1_file):
     with HDF5EventSource(input_url=dl1_file) as source:
         assert source.is_simulation
-        assert source.datamodel_version == (5, 0, 0)
+        assert source.datamodel_version == (5, 1, 0)
         assert set(source.datalevels) == {
             DataLevel.DL1_IMAGES,
             DataLevel.DL1_PARAMETERS,
diff --git a/ctapipe/tools/tests/test_apply_models.py b/ctapipe/tools/tests/test_apply_models.py
index 32b0e4bc..1075cb13 100644
--- a/ctapipe/tools/tests/test_apply_models.py
+++ b/ctapipe/tools/tests/test_apply_models.py
@@ -208,9 +208,8 @@ def test_apply_all(
         assert f"{prefix_disp}_tel_alt" in tel_events.colnames
         assert f"{prefix_disp}_tel_az" in tel_events.colnames
         assert f"{prefix_disp}_tel_is_valid" in tel_events.colnames
-        assert f"{prefix_disp}_parameter_norm" in tel_events.colnames
-        assert f"{prefix_disp}_parameter_is_valid" in tel_events.colnames
-        assert f"{prefix_disp}_parameter_tel_is_valid" not in tel_events.colnames
+        assert f"{prefix_disp}_tel_parameter" in tel_events.colnames
+        assert f"{prefix_disp}_parameter" not in tel_events.colnames
 
         # check that the "--no-dl1-parameters" option worked
         assert "hillas_intensity" not in tel_events.colnames
diff --git a/ctapipe/tools/tests/test_process_ml.py b/ctapipe/tools/tests/test_process_ml.py
index a4fb75d4..6a25f503 100644
--- a/ctapipe/tools/tests/test_process_ml.py
+++ b/ctapipe/tools/tests/test_process_ml.py
@@ -161,8 +161,7 @@ def test_process_apply_disp(
     run_tool(tool, argv=argv, cwd=tmp_path, raises=True)
 
     tel_events = read_table(output, "/dl2/event/telescope/disp/disp/tel_004")
-    assert "disp_parameter_norm" in tel_events.colnames
-    assert "disp_parameter_is_valid" in tel_events.colnames
+    assert "disp_tel_parameter" in tel_events.colnames
 
     tel_events = read_table(output, "/dl2/event/telescope/geometry/disp/tel_004")
     assert "disp_tel_alt" in tel_events.colnames