Repository: cta-observatory/ctapipe (python)

Issue:
`EventSource.simulation_config` should not try to be clever and change type
Currently, `EventSource.simulation_config` is a single simulation config if the source has only one obs id and a dict[obs_id->config] for multiple obs_ids.


This makes it harder to type check and write code for this. 

We should just always use `dict[obs_id -> config]` and let it be length 1 if it only has one entry

Pull request description:
Unify source.simulation_config return value
- HDF5EventSource and SimtelEventSource return a dict
of obs_id -> SimulationConfigContainer independent of the number
of runs in the file
- Removed workaround for the old behaviour in datawriter
- Adapted tests
- Fixes #1621 

Patch:
diff --git a/ctapipe/io/datawriter.py b/ctapipe/io/datawriter.py
index 4ff92efd..9f19e86e 100644
--- a/ctapipe/io/datawriter.py
+++ b/ctapipe/io/datawriter.py
@@ -290,7 +290,7 @@ class DataWriter(Component):
             self._write_dl2_stereo_event(self._writer, event)
 
     def finish(self):
-        """ called after all events are done """
+        """called after all events are done"""
         self.log.info("Finishing DL1 output")
         if not self._at_least_one_event:
             self.log.warning("No events have been written to the output file")
@@ -311,7 +311,7 @@ class DataWriter(Component):
 
     @property
     def datalevels(self):
-        """ returns a list of data levels requested """
+        """returns a list of data levels requested"""
         data_levels = []
         if self.write_images:
             data_levels.append(DataLevel.DL1_IMAGES)
@@ -326,7 +326,7 @@ class DataWriter(Component):
         return data_levels
 
     def _setup_compression(self):
-        """ setup HDF5 compression"""
+        """setup HDF5 compression"""
         self._hdf5_filters = tables.Filters(
             complevel=self.compression_level,
             complib=self.compression_type,
@@ -464,7 +464,7 @@ class DataWriter(Component):
         self.log.debug("Writer initialized: %s", self._writer)
 
     def _write_subarray_pointing(self, event: ArrayEventContainer, writer: TableWriter):
-        """ store subarray pointing info in a monitoring table """
+        """store subarray pointing info in a monitoring table"""
         pnt = event.pointing
         current_pointing = (pnt.array_azimuth, pnt.array_altitude)
         if current_pointing != self._last_pointing:
@@ -487,15 +487,8 @@ class DataWriter(Component):
             container_prefix = ""
             obs_id = Field(0, "Simulation Run Identifier")
 
-        if len(self.event_source.obs_ids) > 1:
-            for obs_id, config in self.event_source.simulation_config.items():
-                extramc = ExtraSimInfo(obs_id=obs_id)
-                config.prefix = ""
-
-                self._writer.write("configuration/simulation/run", [extramc, config])
-        else:
-            extramc = ExtraSimInfo(obs_id=self.event_source.obs_ids[0])
-            config = self.event_source.simulation_config
+        for obs_id, config in self.event_source.simulation_config.items():
+            extramc = ExtraSimInfo(obs_id=obs_id)
             config.prefix = ""
 
             self._writer.write("configuration/simulation/run", [extramc, config])
@@ -526,7 +519,7 @@ class DataWriter(Component):
         def fill_from_simtel(
             obs_id, eventio_hist, container: SimulatedShowerDistribution
         ):
-            """ fill from a SimTel Histogram entry"""
+            """fill from a SimTel Histogram entry"""
             container.obs_id = obs_id
             container.hist_id = eventio_hist["id"]
             container.num_entries = eventio_hist["entries"]
@@ -542,7 +535,7 @@ class DataWriter(Component):
             )
 
             container.bins_core_dist = xbins * u.m
-            container.bins_energy = 10 ** ybins * u.TeV
+            container.bins_energy = 10**ybins * u.TeV
             container.histogram = eventio_hist["data"]
             container.meta["hist_title"] = eventio_hist["title"]
             container.meta["x_label"] = "Log10 E (TeV)"
@@ -718,7 +711,7 @@ class DataWriter(Component):
                 )
 
     def _generate_table_indices(self, h5file, start_node):
-        """ helper to generate PyTables index tabnles for common columns """
+        """helper to generate PyTables index tabnles for common columns"""
         for node in h5file.iter_nodes(start_node):
             if not isinstance(node, tables.group.Group):
                 self.log.debug("generating indices for node: %s", node)
@@ -736,7 +729,7 @@ class DataWriter(Component):
                 self._generate_table_indices(h5file, node)
 
     def _generate_indices(self):
-        """ generate PyTables index tables for common columns """
+        """generate PyTables index tables for common columns"""
         self.log.debug("Writing index tables")
         if self.write_images:
             self._generate_table_indices(
diff --git a/ctapipe/io/hdf5eventsource.py b/ctapipe/io/hdf5eventsource.py
index 1cc873f6..dca222ed 100644
--- a/ctapipe/io/hdf5eventsource.py
+++ b/ctapipe/io/hdf5eventsource.py
@@ -208,11 +208,9 @@ class HDF5EventSource(EventSource):
     @property
     def simulation_config(self):
         """
-        Returns the simulation config.
-        In case of a merged file, this will be a list of simulation configs.
+        Returns the simulation config(s) as
+        a dict mapping obs_id to the respective config.
         """
-        if len(self._simulation_configs) == 1:
-            return next(iter(self._simulation_configs.values()))
         return self._simulation_configs
 
     def _generator(self):
@@ -366,8 +364,10 @@ class HDF5EventSource(EventSource):
             data.count = counter
             data.trigger = trigger
             data.index = index
-            data.trigger.tels_with_trigger = self._full_subarray_info.tel_mask_to_tel_ids(
-                data.trigger.tels_with_trigger
+            data.trigger.tels_with_trigger = (
+                self._full_subarray_info.tel_mask_to_tel_ids(
+                    data.trigger.tels_with_trigger
+                )
             )
             if self.allowed_tels:
                 data.trigger.tels_with_trigger = np.intersect1d(
diff --git a/ctapipe/io/simteleventsource.py b/ctapipe/io/simteleventsource.py
index d3baae36..853a4791 100644
--- a/ctapipe/io/simteleventsource.py
+++ b/ctapipe/io/simteleventsource.py
@@ -42,7 +42,7 @@ from ..instrument.guess import unknown_telescope, guess_telescope
 from .datalevels import DataLevel
 from .eventsource import EventSource
 
-X_MAX_UNIT = u.g / (u.cm ** 2)
+X_MAX_UNIT = u.g / (u.cm**2)
 
 
 __all__ = ["SimTelEventSource"]
@@ -85,7 +85,7 @@ def build_camera(cam_settings, pixel_settings, telescope, frame):
         pix_id=np.arange(cam_settings["n_pixels"]),
         pix_x=u.Quantity(cam_settings["pixel_x"], u.m),
         pix_y=u.Quantity(cam_settings["pixel_y"], u.m),
-        pix_area=u.Quantity(cam_settings["pixel_area"], u.m ** 2),
+        pix_area=u.Quantity(cam_settings["pixel_area"], u.m**2),
         pix_type=pix_type,
         pix_rotation=pix_rotation,
         cam_rotation=-Angle(cam_settings["cam_rot"], u.rad),
@@ -164,7 +164,7 @@ def apply_simtel_r1_calibration(
 
 
 class SimTelEventSource(EventSource):
-    """ Read events from a SimTelArray data file (in EventIO format)."""
+    """Read events from a SimTelArray data file (in EventIO format)."""
 
     skip_calibration_events = Bool(True, help="Skip calibration events").tag(
         config=True
@@ -307,7 +307,7 @@ class SimTelEventSource(EventSource):
 
             n_pixels = cam_settings["n_pixels"]
             focal_length = u.Quantity(cam_settings["focal_length"], u.m)
-            mirror_area = u.Quantity(cam_settings["mirror_area"], u.m ** 2)
+            mirror_area = u.Quantity(cam_settings["mirror_area"], u.m**2)
 
             if self.focal_length_choice == "effective":
                 try:
@@ -568,9 +568,19 @@ class SimTelEventSource(EventSource):
             data.pointing.array_dec = u.Quantity(dec, u.rad)
 
     def _parse_simulation_header(self):
+        """
+        Parse the simulation infos and return a dict with
+        observation ids mapped to SimulationConfigContainers.
+        As merged simtel files are not supported at this
+        point in time, this dictionary will always have
+        length 1.
+        """
+        assert len(self.obs_ids) == 1
+        obs_id = self.obs_ids[0]
+        # With only one run, we can take the first entry:
         mc_run_head = self.file_.mc_run_headers[-1]
 
-        return SimulationConfigContainer(
+        simulation_config = SimulationConfigContainer(
             corsika_version=mc_run_head["shower_prog_vers"],
             simtel_version=mc_run_head["detector_prog_vers"],
             energy_range_min=mc_run_head["E_range"][0] * u.TeV,
@@ -607,6 +617,7 @@ class SimTelEventSource(EventSource):
             corsika_low_E_detail=mc_run_head["corsika_low_E_detail"],
             corsika_high_E_detail=mc_run_head["corsika_high_E_detail"],
         )
+        return {obs_id: simulation_config}
 
     @staticmethod
     def _fill_simulated_event_information(data, array_event):


Test patch:
diff --git a/ctapipe/io/tests/test_hdf5eventsource.py b/ctapipe/io/tests/test_hdf5eventsource.py
index 933b6ddd..cf45bf95 100644
--- a/ctapipe/io/tests/test_hdf5eventsource.py
+++ b/ctapipe/io/tests/test_hdf5eventsource.py
@@ -20,7 +20,7 @@ def test_metadata(dl1_file):
             DataLevel.DL1_PARAMETERS,
         }
         assert list(source.obs_ids) == [2]
-        assert source.simulation_config.corsika_version == 7710
+        assert source.simulation_config[2].corsika_version == 7710
 
 
 def test_subarray(dl1_file):
diff --git a/ctapipe/io/tests/test_simteleventsource.py b/ctapipe/io/tests/test_simteleventsource.py
index 12f110d5..d94a09c6 100644
--- a/ctapipe/io/tests/test_simteleventsource.py
+++ b/ctapipe/io/tests/test_simteleventsource.py
@@ -68,12 +68,16 @@ def test_additional_meta_data_from_simulation_config():
     from astropy import units as u
     from astropy.coordinates import Angle
 
-    assert reader.simulation_config.corsika_version == 6990
-    assert reader.simulation_config.spectral_index == -2.0
-    assert reader.simulation_config.shower_reuse == 20
-    assert reader.simulation_config.core_pos_mode == 1
-    assert reader.simulation_config.diffuse == 1
-    assert reader.simulation_config.atmosphere == 26
+    # There should be only one observation
+    assert len(reader.obs_ids) == 1
+    simulation_config = reader.simulation_config[reader.obs_ids[0]]
+
+    assert simulation_config.corsika_version == 6990
+    assert simulation_config.spectral_index == -2.0
+    assert simulation_config.shower_reuse == 20
+    assert simulation_config.core_pos_mode == 1
+    assert simulation_config.diffuse == 1
+    assert simulation_config.atmosphere == 26
 
     # value read by hand from input card
     name_expectation = {
@@ -91,7 +95,7 @@ def test_additional_meta_data_from_simulation_config():
     }
 
     for name, expectation in name_expectation.items():
-        value = getattr(reader.simulation_config, name)
+        value = getattr(simulation_config, name)
 
         assert value.unit == expectation.unit
         assert np.isclose(
@@ -103,9 +107,9 @@ def test_properties():
     source = SimTelEventSource(input_url=gamma_test_large_path)
 
     assert source.is_simulation
-    assert source.simulation_config.corsika_version == 6990
     assert source.datalevels == (DataLevel.R0, DataLevel.R1)
     assert source.obs_ids == [7514]
+    assert source.simulation_config[7514].corsika_version == 6990
 
 
 def test_gamma_file():