From 9671e470654a09c205e742108c096f00b5322b3a Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Wed, 30 Apr 2025 18:41:03 +0200 Subject: [PATCH 1/8] Start reworking how we serialize ml models/ reconstructors --- src/ctapipe/reco/reconstructor.py | 84 +++++++++--- src/ctapipe/reco/sklearn.py | 135 ++++++++++---------- src/ctapipe/tools/apply_models.py | 8 +- src/ctapipe/tools/train_energy_regressor.py | 20 ++- 4 files changed, 157 insertions(+), 90 deletions(-) diff --git a/src/ctapipe/reco/reconstructor.py b/src/ctapipe/reco/reconstructor.py index d09c7d4b362..d2ae54e7685 100644 --- a/src/ctapipe/reco/reconstructor.py +++ b/src/ctapipe/reco/reconstructor.py @@ -1,3 +1,4 @@ +import pathlib import weakref from abc import abstractmethod from enum import Flag, auto @@ -8,8 +9,8 @@ from astropy.coordinates import AltAz, SkyCoord from ctapipe.containers import ArrayEventContainer, TelescopeImpactParameterContainer -from ctapipe.core import Provenance, QualityQuery, TelescopeComponent -from ctapipe.core.traits import Integer, List +from ctapipe.core import Component, Provenance, QualityQuery, TelescopeComponent +from ctapipe.core.traits import Integer, List, Path from ..coordinates import shower_impact_distance @@ -84,10 +85,24 @@ class Reconstructor(TelescopeComponent): help="Number of threads to use for the reconstruction if supported by the reconstructor.", ).tag(config=True) + load_path = Path( + default_value=None, + allow_none=True, + help="If given, load serialized model from this path.", + ).tag(config=True) + def __init__(self, subarray, atmosphere_profile=None, **kwargs): - super().__init__(subarray=subarray, **kwargs) - self.quality_query = StereoQualityQuery(parent=self) - self.atmosphere_profile = atmosphere_profile + # Run the Component __init__ first to handle the configuration + # and make `self.load_path` available + Component.__init__(self, **kwargs) + + if self.load_path is None: + self.subarray = subarray + self.quality_query = StereoQualityQuery(parent=self) + self.atmosphere_profile = atmosphere_profile + else: + loaded = self.read(self.load_path, subarray=subarray, **kwargs) + self.__dict__.update(loaded.__dict__) @abstractmethod def __call__(self, event: ArrayEventContainer): @@ -105,40 +120,72 @@ def __call__(self, event: ArrayEventContainer): reconstructed stereo geometry and telescope-wise impact position. """ + def write(self, dictionary, path, overwrite=False): + """ + Save a dictionary using joblib-pickle, which should contain all + information/settings about an instance of a reconstructor (subclass). + + Parameters + ---------- + dictionary : dict + Dictionary to be saved. It can contain as many entries as needed, + but must at least include the following: + "name": Name of the ``Reconstructor`` subclass, + "meta": Additional metadata + path : str or pathlib.Path + Path to which the dictionary will be saved. + overwrite : Bool + Whether to overwrite, if ``path`` already exists. + """ + path = pathlib.Path(path) + + if path.exists() and not overwrite: + raise OSError(f"Path {path} exists and overwrite=False") + + with path.open("wb") as f: + Provenance().add_output_file(path, role="reconstructor") + joblib.dump(dictionary, f, compress=True) + @classmethod def read(cls, path, parent=None, subarray=None, **kwargs): - """Read a joblib-pickled reconstructor from ``path`` + """ + Read a dictionary from ``path`` containing all necessary information + to construct an instance of a reconstructor (subclass). Parameters ---------- path : str or pathlib.Path - Path to a Reconstructor instance pickled using joblib + Path to a dictionary containing all information about a + ``Reconstructor`` (subclass). parent : None or Component or Tool - Attach a new parent to the loaded class, this will properly + Attach a new parent to the loaded class. subarray : SubarrayDescription Attach a new subarray to the loaded reconstructor A warning will be raised if the telescope types of the subarray stored in the pickled class do not match with the provided subarray. - **kwargs are set on the loaded instance + **kwargs are set on the constructed instance Returns ------- - Reconstructor instance loaded from file + Reconstructor instance """ with open(path, "rb") as f: - instance = joblib.load(f) + dictionary = joblib.load(f) - if not isinstance(instance, cls): - raise TypeError( - f"{path} did not contain an instance of {cls}, got {instance}" - ) + meta = dictionary.pop("meta") + name = dictionary.pop("name") + loaded_subarray = dictionary.pop("subarray") + instance = Reconstructor.from_name(name, subarray=loaded_subarray) + + for attr, value in dictionary.items(): + setattr(instance, attr, value) - # first deal with kwargs that would need "special" treatmet, parent and subarray + # first deal with kwargs that would need "special" treatment, parent and subarray if parent is not None: instance.parent = weakref.proxy(parent) - instance.log = parent.log.getChild(instance.__class__.__name__) + instance.log = parent.log.getChild(name) if subarray is not None: if instance.subarray.telescope_types != subarray.telescope_types: @@ -150,8 +197,7 @@ def read(cls, path, parent=None, subarray=None, **kwargs): for attr, value in kwargs.items(): setattr(instance, attr, value) - # FIXME: we currently don't store metadata in the joblib / pickle files, see #2603 - Provenance().add_input_file(path, role="reconstructor", add_meta=False) + Provenance().add_input_file(path, role="reconstructor", reference_meta=meta) return instance diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index acc77e964ad..39c5adb9dae 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -1,7 +1,8 @@ """ Component Wrappers around sklearn models """ -import pathlib + +import weakref from abc import abstractmethod from collections import defaultdict from copy import deepcopy @@ -117,12 +118,6 @@ class SKLearnReconstructor(Reconstructor): help="Which stereo combination method to use.", ).tag(config=True) - load_path = traits.Path( - default_value=None, - allow_none=True, - help="If given, load serialized model from this path.", - ).tag(config=True) - def __init__( self, subarray=None, atmosphere_profile=None, models=None, n_jobs=None, **kwargs ): @@ -162,15 +157,7 @@ def __init__( ) else: loaded = self.read(self.load_path) - if ( - subarray is not None - and loaded.subarray.telescope_types != subarray.telescope_types - ): - self.log.warning( - "Supplied subarray has different telescopes than subarray loaded from file" - ) self.__dict__.update(loaded.__dict__) - self.subarray = subarray if self.prefix is None: self.prefix = self.model_cls @@ -207,16 +194,6 @@ def predict_table(self, key, table: Table) -> Table: container definition(s) """ - def write(self, path, overwrite=False): - path = pathlib.Path(path) - - if path.exists() and not overwrite: - raise OSError(f"Path {path} exists and overwrite=False") - - with path.open("wb") as f: - Provenance().add_output_file(path, role="ml-models") - joblib.dump(self, f, compress=True) - @lazyproperty def instrument_table(self): return QTable(self.subarray.to_table("joined")) @@ -257,6 +234,70 @@ def _set_n_jobs(self, n_jobs): for model in self._models.values(): model.n_jobs = n_jobs.new + @classmethod + def read(cls, path, parent=None, subarray=None, **kwargs): + """ + Read a dictionary from ``path`` containing all necessary information + to construct an instance of a ``SKLearnReconstructor`` (subclass). + + Parameters + ---------- + path : str or pathlib.Path + Path to a dictionary containing all information about a + ``SKLearnReconstructor`` (subclass). + parent : None or Component or Tool + Attach a new parent to the loaded class. + subarray : SubarrayDescription + Attach a new subarray to the loaded reconstructor + A warning will be raised if the telescope types of the + subarray stored in the pickled class do not match with the + provided subarray. + + **kwargs are set on the constructed instance + + Returns + ------- + ``SKLearnReconstructor`` (subclass) instance + """ + # Overloading Reconstructor.read() is necessary here, + # because model_cls and model_config are needed for __init__ + # to verify that these settings are valid. + with open(path, "rb") as f: + dictionary = joblib.load(f) + + meta = dictionary.pop("meta") + name = dictionary.pop("name") + loaded_subarray = dictionary.pop("subarray") + model_cls = dictionary.pop("model_cls") + model_config = dictionary.pop("model_config") + instance = SKLearnReconstructor.from_name( + name=name, + subarray=loaded_subarray, + model_cls=model_cls, + model_config=model_config, + ) + + for attr, value in dictionary.items(): + setattr(instance, attr, value) + + # first deal with kwargs that would need "special" treatment, parent and subarray + if parent is not None: + instance.parent = weakref.proxy(parent) + instance.log = parent.log.getChild(name) + + if subarray is not None: + if instance.subarray.telescope_types != subarray.telescope_types: + instance.log.warning( + "Supplied subarray has different telescopes than subarray loaded from file" + ) + instance.subarray = subarray + + for attr, value in kwargs.items(): + setattr(instance, attr, value) + + Provenance().add_input_file(path, role="reconstructor", reference_meta=meta) + return instance + class SKLearnRegressionReconstructor(SKLearnReconstructor): """Base class for regression tasks.""" @@ -566,12 +607,6 @@ class DispReconstructor(Reconstructor): help="Which stereo combination method to use.", ).tag(config=True) - load_path = traits.Path( - default_value=None, - allow_none=True, - help="If given, load serialized model from this path.", - ).tag(config=True) - def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs): # Run the Component __init__ first to handle the configuration # and make `self.load_path` available @@ -604,15 +639,10 @@ def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs ) else: loaded = self.read(self.load_path) - if ( - subarray is not None - and loaded.subarray.telescope_types != subarray.telescope_types - ): - self.log.warning( - "Supplied subarray has different telescopes than subarray loaded from file" - ) self.__dict__.update(loaded.__dict__) - self.subarray = subarray + + if self.prefix is None: + self.prefix = "disp" def _new_models(self): norm_cfg = self.norm_config @@ -654,33 +684,6 @@ def fit(self, key, table): self._models[key][0].fit(X, norm) self._models[key][1].fit(X, sign) - def write(self, path, overwrite=False): - path = pathlib.Path(path) - - if path.exists() and not overwrite: - raise OSError(f"Path {path} exists and overwrite=False") - - with path.open("wb") as f: - Provenance().add_output_file(path, role="ml-models") - joblib.dump(self, f, compress=True) - - @classmethod - def read(cls, path, **kwargs): - with open(path, "rb") as f: - instance = joblib.load(f) - - for attr, value in kwargs.items(): - setattr(instance, attr, value) - - if not isinstance(instance, cls): - raise TypeError( - f"{path} did not contain an instance of {cls}, got {instance}" - ) - - # FIXME: we currently don't store metadata in the joblib / pickle files, see #2603 - Provenance().add_input_file(path, role="ml-models", add_meta=False) - return instance - @lazyproperty def instrument_table(self): return self.subarray.to_table("joined") diff --git a/src/ctapipe/tools/apply_models.py b/src/ctapipe/tools/apply_models.py index 3caff55fed1..4153974c1d5 100644 --- a/src/ctapipe/tools/apply_models.py +++ b/src/ctapipe/tools/apply_models.py @@ -12,7 +12,7 @@ from ctapipe.io import HDF5Merger, TableLoader, write_table from ctapipe.io.astropy_helpers import join_allow_empty, read_table from ctapipe.io.tableio import TelListToMaskTransform -from ctapipe.reco import Reconstructor +from ctapipe.reco.sklearn import SKLearnReconstructor __all__ = [ "ApplyModels", @@ -127,7 +127,7 @@ class ApplyModels(Tool): ), } - classes = [TableLoader] + classes_with_traits(Reconstructor) + classes = [TableLoader] + classes_with_traits(SKLearnReconstructor) def setup(self): """ @@ -149,7 +149,9 @@ def setup(self): self._reconstructors = [] for path in self.reconstructor_paths: - r = Reconstructor.read(path, parent=self, subarray=self.loader.subarray) + r = SKLearnReconstructor.read( + path, parent=self, subarray=self.loader.subarray + ) if self.n_jobs: r.n_jobs = self.n_jobs self._reconstructors.append(r) diff --git a/src/ctapipe/tools/train_energy_regressor.py b/src/ctapipe/tools/train_energy_regressor.py index 408e1ed7d51..d6e0e77bcfb 100644 --- a/src/ctapipe/tools/train_energy_regressor.py +++ b/src/ctapipe/tools/train_energy_regressor.py @@ -1,6 +1,7 @@ """ Tool for training the EnergyRegressor """ + import numpy as np from ctapipe.core import Tool @@ -141,8 +142,23 @@ def finish(self): Write-out trained models and cross-validation results. """ self.log.info("Writing output") - self.regressor.n_jobs = None - self.regressor.write(self.output_path, overwrite=self.overwrite) + dictionary = { + "name": "EnergyRegressor", + "subarray": self.regressor.subarray, + "prefix": self.regressor.prefix, + "property": self.regressor.property, + "target": self.regressor.target, + "log_target": self.regressor.log_target, + "features": self.regressor.features, + "model_cls": self.regressor.model_cls, + "model_config": self.regressor.model_config, + "models": self.regressor._models, + "stereo_combiner_cls": self.regressor.stereo_combiner_cls, + "feature_generator": self.regressor.feature_generator, + "quality_query": self.regressor.quality_query, + "meta": {}, + } + self.regressor.write(dictionary, self.output_path, overwrite=self.overwrite) self.loader.close() self.cross_validate.close() From 5fb8fa60ea2f1ccbc83a1934a20e440b1b811bb7 Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Wed, 7 May 2025 18:54:42 +0200 Subject: [PATCH 2/8] Gets it working --- src/ctapipe/reco/reconstructor.py | 43 +--- src/ctapipe/reco/sklearn.py | 204 ++++++++++++------ src/ctapipe/tools/apply_models.py | 8 +- src/ctapipe/tools/train_disp_reconstructor.py | 1 - src/ctapipe/tools/train_energy_regressor.py | 18 +- .../tools/train_particle_classifier.py | 2 +- 6 files changed, 150 insertions(+), 126 deletions(-) diff --git a/src/ctapipe/reco/reconstructor.py b/src/ctapipe/reco/reconstructor.py index d2ae54e7685..5a3e386e2e7 100644 --- a/src/ctapipe/reco/reconstructor.py +++ b/src/ctapipe/reco/reconstructor.py @@ -1,4 +1,3 @@ -import pathlib import weakref from abc import abstractmethod from enum import Flag, auto @@ -120,32 +119,6 @@ def __call__(self, event: ArrayEventContainer): reconstructed stereo geometry and telescope-wise impact position. """ - def write(self, dictionary, path, overwrite=False): - """ - Save a dictionary using joblib-pickle, which should contain all - information/settings about an instance of a reconstructor (subclass). - - Parameters - ---------- - dictionary : dict - Dictionary to be saved. It can contain as many entries as needed, - but must at least include the following: - "name": Name of the ``Reconstructor`` subclass, - "meta": Additional metadata - path : str or pathlib.Path - Path to which the dictionary will be saved. - overwrite : Bool - Whether to overwrite, if ``path`` already exists. - """ - path = pathlib.Path(path) - - if path.exists() and not overwrite: - raise OSError(f"Path {path} exists and overwrite=False") - - with path.open("wb") as f: - Provenance().add_output_file(path, role="reconstructor") - joblib.dump(dictionary, f, compress=True) - @classmethod def read(cls, path, parent=None, subarray=None, **kwargs): """ @@ -174,18 +147,24 @@ def read(cls, path, parent=None, subarray=None, **kwargs): with open(path, "rb") as f: dictionary = joblib.load(f) + if dictionary["name"] != cls.__name__: + raise TypeError( + f"{path} does not contain information about {cls.__name__}, " + f"but instead about {dictionary['name']}." + ) + meta = dictionary.pop("meta") - name = dictionary.pop("name") - loaded_subarray = dictionary.pop("subarray") - instance = Reconstructor.from_name(name, subarray=loaded_subarray) + cls_attributes = dictionary.pop("cls_attributes") + instance = Reconstructor.from_name(**dictionary) - for attr, value in dictionary.items(): + # set class attributes not handled by __init__ + for attr, value in cls_attributes.items(): setattr(instance, attr, value) # first deal with kwargs that would need "special" treatment, parent and subarray if parent is not None: instance.parent = weakref.proxy(parent) - instance.log = parent.log.getChild(name) + instance.log = parent.log.getChild(dictionary["name"]) if subarray is not None: if instance.subarray.telescope_types != subarray.telescope_types: diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index 39c5adb9dae..5d0dc9659d0 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -2,7 +2,7 @@ Component Wrappers around sklearn models """ -import weakref +import pathlib from abc import abstractmethod from collections import defaultdict from copy import deepcopy @@ -118,9 +118,7 @@ class SKLearnReconstructor(Reconstructor): help="Which stereo combination method to use.", ).tag(config=True) - def __init__( - self, subarray=None, atmosphere_profile=None, models=None, n_jobs=None, **kwargs - ): + def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs): # Run the Component __init__ first to handle the configuration # and make `self.load_path` available Component.__init__(self, **kwargs) @@ -175,7 +173,7 @@ def __call__(self, event: ArrayEventContainer) -> None: """ @abstractmethod - def predict_table(self, key, table: Table) -> Table: + def predict_table(self, key, table: Table) -> dict[ReconstructionProperty, Table]: """ Predict on a table of events. @@ -234,70 +232,6 @@ def _set_n_jobs(self, n_jobs): for model in self._models.values(): model.n_jobs = n_jobs.new - @classmethod - def read(cls, path, parent=None, subarray=None, **kwargs): - """ - Read a dictionary from ``path`` containing all necessary information - to construct an instance of a ``SKLearnReconstructor`` (subclass). - - Parameters - ---------- - path : str or pathlib.Path - Path to a dictionary containing all information about a - ``SKLearnReconstructor`` (subclass). - parent : None or Component or Tool - Attach a new parent to the loaded class. - subarray : SubarrayDescription - Attach a new subarray to the loaded reconstructor - A warning will be raised if the telescope types of the - subarray stored in the pickled class do not match with the - provided subarray. - - **kwargs are set on the constructed instance - - Returns - ------- - ``SKLearnReconstructor`` (subclass) instance - """ - # Overloading Reconstructor.read() is necessary here, - # because model_cls and model_config are needed for __init__ - # to verify that these settings are valid. - with open(path, "rb") as f: - dictionary = joblib.load(f) - - meta = dictionary.pop("meta") - name = dictionary.pop("name") - loaded_subarray = dictionary.pop("subarray") - model_cls = dictionary.pop("model_cls") - model_config = dictionary.pop("model_config") - instance = SKLearnReconstructor.from_name( - name=name, - subarray=loaded_subarray, - model_cls=model_cls, - model_config=model_config, - ) - - for attr, value in dictionary.items(): - setattr(instance, attr, value) - - # first deal with kwargs that would need "special" treatment, parent and subarray - if parent is not None: - instance.parent = weakref.proxy(parent) - instance.log = parent.log.getChild(name) - - if subarray is not None: - if instance.subarray.telescope_types != subarray.telescope_types: - instance.log.warning( - "Supplied subarray has different telescopes than subarray loaded from file" - ) - instance.subarray = subarray - - for attr, value in kwargs.items(): - setattr(instance, attr, value) - - Provenance().add_input_file(path, role="reconstructor", reference_meta=meta) - return instance - class SKLearnRegressionReconstructor(SKLearnReconstructor): """Base class for regression tasks.""" @@ -352,6 +286,49 @@ def _table_to_y(self, table, mask=None): return np.log(y) return y + def write(self, path, meta={}, overwrite=False): + """ + Save a dictionary using joblib-pickle, which contains all + information/settings about an instance of a + ``SKLearnRegressionReconstructor`` (subclass). + + Parameters + ---------- + path : str or pathlib.Path + Path to which the dictionary will be saved. + meta : dict + Metadata + overwrite : Bool + Whether to overwrite, if ``path`` already exists. + """ + path = pathlib.Path(path) + + if path.exists() and not overwrite: + raise OSError(f"Path {path} exists and overwrite=False") + + dictionary = { + "name": self.__class__.__name__, + "subarray": self.subarray, + "prefix": self.prefix, + "log_target": self.log_target, + "features": self.features, + "model_cls": self.model_cls, + "model_config": self.model_config, + "models": self._models, + "stereo_combiner_cls": self.stereo_combiner_cls, + "cls_attributes": { + "property": self.property, + "target": self.target, + "unit": self.unit, + "feature_generator": self.feature_generator, + "quality_query": self.quality_query, + }, + "meta": meta, + } + with path.open("wb") as f: + Provenance().add_output_file(path, role="ml-reconstructor") + joblib.dump(dictionary, f, compress=True) + class SKLearnClassificationReconstructor(SKLearnReconstructor): """Base class for classification tasks.""" @@ -427,6 +404,48 @@ def _predict_score(self, key, table): def _get_positive_index(self, key): return np.nonzero(self._models[key].classes_ == self.positive_class)[0][0] + def write(self, path, meta={}, overwrite=False): + """ + Save a dictionary using joblib-pickle, which contains all + information/settings about an instance of a + ``SKLearnClassificationReconstructor`` (subclass). + + Parameters + ---------- + path : str or pathlib.Path + Path to which the dictionary will be saved. + meta : dict + Metadata + overwrite : Bool + Whether to overwrite, if ``path`` already exists. + """ + path = pathlib.Path(path) + + if path.exists() and not overwrite: + raise OSError(f"Path {path} exists and overwrite=False") + + dictionary = { + "name": self.__class__.__name__, + "subarray": self.subarray, + "prefix": self.prefix, + "positive_class": self.positive_class, + "features": self.features, + "model_cls": self.model_cls, + "model_config": self.model_config, + "models": self._models, + "stereo_combiner_cls": self.stereo_combiner_cls, + "cls_attributes": { + "property": self.property, + "target": self.target, + "feature_generator": self.feature_generator, + "quality_query": self.quality_query, + }, + "meta": meta, + } + with path.open("wb") as f: + Provenance().add_output_file(path, role="ml-reconstructor") + joblib.dump(dictionary, f, compress=True) + class EnergyRegressor(SKLearnRegressionReconstructor): """ @@ -559,6 +578,7 @@ class DispReconstructor(Reconstructor): """ target = "true_disp" + property = ReconstructionProperty.GEOMETRY prefix = traits.Unicode( default_value="disp", @@ -634,7 +654,7 @@ def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs self.stereo_combiner = StereoCombiner.from_name( self.stereo_combiner_cls, prefix=self.prefix, - property=ReconstructionProperty.GEOMETRY, + property=self.property, parent=self, ) else: @@ -873,6 +893,50 @@ def _set_n_jobs(self, n_jobs): disp.n_jobs = n_jobs.new sign.n_jobs = n_jobs.new + def write(self, path, meta={}, overwrite=False): + """ + Save a dictionary using joblib-pickle, which contains all + information/settings about an instance of a ``DispReconstructor`` . + + Parameters + ---------- + path : str or pathlib.Path + Path to which the dictionary will be saved. + meta : dict + Metadata + overwrite : Bool + Whether to overwrite, if ``path`` already exists. + """ + path = pathlib.Path(path) + + if path.exists() and not overwrite: + raise OSError(f"Path {path} exists and overwrite=False") + + dictionary = { + "name": self.__class__.__name__, + "subarray": self.subarray, + "prefix": self.prefix, + "log_target": self.log_target, + "features": self.features, + "norm_cls": self.norm_cls, + "sign_cls": self.sign_cls, + "norm_config": self.norm_config, + "sign_config": self.sign_config, + "models": self._models, + "stereo_combiner_cls": self.stereo_combiner_cls, + "cls_attributes": { + "property": self.property, + "target": self.target, + "unit": self.unit, + "feature_generator": self.feature_generator, + "quality_query": self.quality_query, + }, + "meta": meta, + } + with path.open("wb") as f: + Provenance().add_output_file(path, role="ml-reconstructor") + joblib.dump(dictionary, f, compress=True) + class CrossValidator(Component): """Class to train sklearn based reconstructors in a cross validation.""" diff --git a/src/ctapipe/tools/apply_models.py b/src/ctapipe/tools/apply_models.py index 4153974c1d5..3caff55fed1 100644 --- a/src/ctapipe/tools/apply_models.py +++ b/src/ctapipe/tools/apply_models.py @@ -12,7 +12,7 @@ from ctapipe.io import HDF5Merger, TableLoader, write_table from ctapipe.io.astropy_helpers import join_allow_empty, read_table from ctapipe.io.tableio import TelListToMaskTransform -from ctapipe.reco.sklearn import SKLearnReconstructor +from ctapipe.reco import Reconstructor __all__ = [ "ApplyModels", @@ -127,7 +127,7 @@ class ApplyModels(Tool): ), } - classes = [TableLoader] + classes_with_traits(SKLearnReconstructor) + classes = [TableLoader] + classes_with_traits(Reconstructor) def setup(self): """ @@ -149,9 +149,7 @@ def setup(self): self._reconstructors = [] for path in self.reconstructor_paths: - r = SKLearnReconstructor.read( - path, parent=self, subarray=self.loader.subarray - ) + r = Reconstructor.read(path, parent=self, subarray=self.loader.subarray) if self.n_jobs: r.n_jobs = self.n_jobs self._reconstructors.append(r) diff --git a/src/ctapipe/tools/train_disp_reconstructor.py b/src/ctapipe/tools/train_disp_reconstructor.py index a125ff753cf..8435735db7f 100644 --- a/src/ctapipe/tools/train_disp_reconstructor.py +++ b/src/ctapipe/tools/train_disp_reconstructor.py @@ -186,7 +186,6 @@ def finish(self): Write-out trained models and cross-validation results. """ self.log.info("Writing output") - self.models.n_jobs = None self.models.write(self.output_path, overwrite=self.overwrite) self.loader.close() self.cross_validate.close() diff --git a/src/ctapipe/tools/train_energy_regressor.py b/src/ctapipe/tools/train_energy_regressor.py index d6e0e77bcfb..87773279fe2 100644 --- a/src/ctapipe/tools/train_energy_regressor.py +++ b/src/ctapipe/tools/train_energy_regressor.py @@ -142,23 +142,7 @@ def finish(self): Write-out trained models and cross-validation results. """ self.log.info("Writing output") - dictionary = { - "name": "EnergyRegressor", - "subarray": self.regressor.subarray, - "prefix": self.regressor.prefix, - "property": self.regressor.property, - "target": self.regressor.target, - "log_target": self.regressor.log_target, - "features": self.regressor.features, - "model_cls": self.regressor.model_cls, - "model_config": self.regressor.model_config, - "models": self.regressor._models, - "stereo_combiner_cls": self.regressor.stereo_combiner_cls, - "feature_generator": self.regressor.feature_generator, - "quality_query": self.regressor.quality_query, - "meta": {}, - } - self.regressor.write(dictionary, self.output_path, overwrite=self.overwrite) + self.regressor.write(self.output_path, overwrite=self.overwrite) self.loader.close() self.cross_validate.close() diff --git a/src/ctapipe/tools/train_particle_classifier.py b/src/ctapipe/tools/train_particle_classifier.py index 7b235ab0bae..c74278df26f 100644 --- a/src/ctapipe/tools/train_particle_classifier.py +++ b/src/ctapipe/tools/train_particle_classifier.py @@ -1,6 +1,7 @@ """ Tool for training the ParticleClassifier """ + import numpy as np from astropy.table import vstack @@ -232,7 +233,6 @@ def finish(self): Write-out trained models and cross-validation results. """ self.log.info("Writing output") - self.classifier.n_jobs = None self.classifier.write(self.output_path, overwrite=self.overwrite) self.signal_loader.close() self.background_loader.close() From c51bd1610d50d48b18f14611b57bcb60528c2db4 Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Mon, 19 May 2025 13:23:13 +0200 Subject: [PATCH 3/8] Allow reading of subclasses --- src/ctapipe/reco/reconstructor.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/ctapipe/reco/reconstructor.py b/src/ctapipe/reco/reconstructor.py index 5a3e386e2e7..c3f945f7978 100644 --- a/src/ctapipe/reco/reconstructor.py +++ b/src/ctapipe/reco/reconstructor.py @@ -9,7 +9,7 @@ from ctapipe.containers import ArrayEventContainer, TelescopeImpactParameterContainer from ctapipe.core import Component, Provenance, QualityQuery, TelescopeComponent -from ctapipe.core.traits import Integer, List, Path +from ctapipe.core.traits import Integer, List, Path, classes_with_traits from ..coordinates import shower_impact_distance @@ -147,10 +147,11 @@ def read(cls, path, parent=None, subarray=None, **kwargs): with open(path, "rb") as f: dictionary = joblib.load(f) - if dictionary["name"] != cls.__name__: + if dictionary["name"] not in [c.__name__ for c in classes_with_traits(cls)]: raise TypeError( - f"{path} does not contain information about {cls.__name__}, " - f"but instead about {dictionary['name']}." + f"{path} does not contain information about {cls.__name__} or " + "one of its subclasses, but instead about " + f"{dictionary['name']}." ) meta = dictionary.pop("meta") From ca0c453567aca2a05127b84d06e8dd30bca9a14f Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Mon, 19 May 2025 15:06:13 +0200 Subject: [PATCH 4/8] Do not serialize instances of FeatureGenerator and QualityQuery --- src/ctapipe/reco/reconstructor.py | 24 ++++++--- src/ctapipe/reco/sklearn.py | 88 +++++++++++++++++-------------- 2 files changed, 65 insertions(+), 47 deletions(-) diff --git a/src/ctapipe/reco/reconstructor.py b/src/ctapipe/reco/reconstructor.py index c3f945f7978..99e1ab9fc90 100644 --- a/src/ctapipe/reco/reconstructor.py +++ b/src/ctapipe/reco/reconstructor.py @@ -6,6 +6,7 @@ import joblib import numpy as np from astropy.coordinates import AltAz, SkyCoord +from traitlets.config import Config from ctapipe.containers import ArrayEventContainer, TelescopeImpactParameterContainer from ctapipe.core import Component, Provenance, QualityQuery, TelescopeComponent @@ -147,25 +148,32 @@ def read(cls, path, parent=None, subarray=None, **kwargs): with open(path, "rb") as f: dictionary = joblib.load(f) - if dictionary["name"] not in [c.__name__ for c in classes_with_traits(cls)]: + name = dictionary.pop("name") + config = Config(dictionary.pop("config")) + cls_attributes = dictionary.pop("cls_attributes") + meta = dictionary.pop("meta") + + if name not in [c.__name__ for c in classes_with_traits(cls)]: raise TypeError( f"{path} does not contain information about {cls.__name__} or " - "one of its subclasses, but instead about " - f"{dictionary['name']}." + f"one of its subclasses, but instead about {name}." ) - meta = dictionary.pop("meta") - cls_attributes = dictionary.pop("cls_attributes") - instance = Reconstructor.from_name(**dictionary) + instance = Reconstructor.from_name( + name=name, + config=config, + **dictionary, + ) - # set class attributes not handled by __init__ + # set class attributes not handled by __init__, + # e.g. the unit defined during SKLearnReconstructor.fit() for attr, value in cls_attributes.items(): setattr(instance, attr, value) # first deal with kwargs that would need "special" treatment, parent and subarray if parent is not None: instance.parent = weakref.proxy(parent) - instance.log = parent.log.getChild(dictionary["name"]) + instance.log = parent.log.getChild(name) if subarray is not None: if instance.subarray.telescope_types != subarray.telescope_types: diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index 5d0dc9659d0..6ac8a85b518 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -309,20 +309,23 @@ def write(self, path, meta={}, overwrite=False): dictionary = { "name": self.__class__.__name__, "subarray": self.subarray, - "prefix": self.prefix, - "log_target": self.log_target, - "features": self.features, - "model_cls": self.model_cls, - "model_config": self.model_config, "models": self._models, - "stereo_combiner_cls": self.stereo_combiner_cls, - "cls_attributes": { - "property": self.property, - "target": self.target, - "unit": self.unit, - "feature_generator": self.feature_generator, - "quality_query": self.quality_query, + "config": { + self.__class__.__name__: { + "prefix": self.prefix, + "log_target": self.log_target, + "model_cls": self.model_cls, + "model_config": self.model_config, + "features": self.features, + "stereo_combiner_cls": self.stereo_combiner_cls, + "FeatureGenerator": {"features": self.feature_generator.features}, + "QualityQuery": { + "quality_criteria": self.quality_query.quality_criteria + }, + self.stereo_combiner_cls: {"weights": self.stereo_combiner.weights}, + } }, + "cls_attributes": {"unit": self.unit}, "meta": meta, } with path.open("wb") as f: @@ -427,19 +430,24 @@ def write(self, path, meta={}, overwrite=False): dictionary = { "name": self.__class__.__name__, "subarray": self.subarray, - "prefix": self.prefix, - "positive_class": self.positive_class, - "features": self.features, - "model_cls": self.model_cls, - "model_config": self.model_config, "models": self._models, - "stereo_combiner_cls": self.stereo_combiner_cls, - "cls_attributes": { - "property": self.property, - "target": self.target, - "feature_generator": self.feature_generator, - "quality_query": self.quality_query, + "config": { + self.__class__.__name__: { + "prefix": self.prefix, + "invalid_class": self.invalid_class, + "positive_class": self.positive_class, + "model_cls": self.model_cls, + "model_config": self.model_config, + "features": self.features, + "stereo_combiner_cls": self.stereo_combiner_cls, + "FeatureGenerator": {"features": self.feature_generator.features}, + "QualityQuery": { + "quality_criteria": self.quality_query.quality_criteria + }, + self.stereo_combiner_cls: {"weights": self.stereo_combiner.weights}, + } }, + "cls_attributes": {"unit": self.unit}, "meta": meta, } with path.open("wb") as f: @@ -512,14 +520,13 @@ class ParticleClassifier(SKLearnClassificationReconstructor): """Predict dl2 particle classification.""" target = "true_shower_primary_id" + property = ReconstructionProperty.PARTICLE_TYPE positive_class = traits.Integer( default_value=0, help="Particle id (in simtel system) of the positive class. Default is 0 for gammas.", ).tag(config=True) - property = ReconstructionProperty.PARTICLE_TYPE - def __call__(self, event: ArrayEventContainer) -> None: for tel_id in event.trigger.tels_with_trigger: table = collect_features(event, tel_id, self.instrument_table) @@ -915,22 +922,25 @@ def write(self, path, meta={}, overwrite=False): dictionary = { "name": self.__class__.__name__, "subarray": self.subarray, - "prefix": self.prefix, - "log_target": self.log_target, - "features": self.features, - "norm_cls": self.norm_cls, - "sign_cls": self.sign_cls, - "norm_config": self.norm_config, - "sign_config": self.sign_config, "models": self._models, - "stereo_combiner_cls": self.stereo_combiner_cls, - "cls_attributes": { - "property": self.property, - "target": self.target, - "unit": self.unit, - "feature_generator": self.feature_generator, - "quality_query": self.quality_query, + "config": { + self.__class__.__name__: { + "prefix": self.prefix, + "log_target": self.log_target, + "norm_cls": self.norm_cls, + "sign_cls": self.sign_cls, + "norm_config": self.norm_config, + "sign_config": self.sign_config, + "features": self.features, + "stereo_combiner_cls": self.stereo_combiner_cls, + "FeatureGenerator": {"features": self.feature_generator.features}, + "QualityQuery": { + "quality_criteria": self.quality_query.quality_criteria + }, + self.stereo_combiner_cls: {"weights": self.stereo_combiner.weights}, + } }, + "cls_attributes": {"unit": self.unit}, "meta": meta, } with path.open("wb") as f: From a7bb6291a9c1f9e9730a6143c13f6635c2ac1f97 Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Thu, 12 Jun 2025 15:44:23 +0200 Subject: [PATCH 5/8] Works as intended now --- src/ctapipe/reco/reconstructor.py | 86 +++++++++++++++++++------------ src/ctapipe/reco/sklearn.py | 12 +++++ 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/src/ctapipe/reco/reconstructor.py b/src/ctapipe/reco/reconstructor.py index 99e1ab9fc90..ac662e790f3 100644 --- a/src/ctapipe/reco/reconstructor.py +++ b/src/ctapipe/reco/reconstructor.py @@ -1,4 +1,3 @@ -import weakref from abc import abstractmethod from enum import Flag, auto @@ -9,8 +8,13 @@ from traitlets.config import Config from ctapipe.containers import ArrayEventContainer, TelescopeImpactParameterContainer -from ctapipe.core import Component, Provenance, QualityQuery, TelescopeComponent -from ctapipe.core.traits import Integer, List, Path, classes_with_traits +from ctapipe.core import ( + Provenance, + QualityQuery, + TelescopeComponent, + ToolConfigurationError, +) +from ctapipe.core.traits import Integer, List, classes_with_traits from ..coordinates import shower_impact_distance @@ -85,24 +89,10 @@ class Reconstructor(TelescopeComponent): help="Number of threads to use for the reconstruction if supported by the reconstructor.", ).tag(config=True) - load_path = Path( - default_value=None, - allow_none=True, - help="If given, load serialized model from this path.", - ).tag(config=True) - def __init__(self, subarray, atmosphere_profile=None, **kwargs): - # Run the Component __init__ first to handle the configuration - # and make `self.load_path` available - Component.__init__(self, **kwargs) - - if self.load_path is None: - self.subarray = subarray - self.quality_query = StereoQualityQuery(parent=self) - self.atmosphere_profile = atmosphere_profile - else: - loaded = self.read(self.load_path, subarray=subarray, **kwargs) - self.__dict__.update(loaded.__dict__) + super().__init__(subarray=subarray, **kwargs) + self.quality_query = StereoQualityQuery(parent=self) + self.atmosphere_profile = atmosphere_profile @abstractmethod def __call__(self, event: ArrayEventContainer): @@ -148,10 +138,10 @@ def read(cls, path, parent=None, subarray=None, **kwargs): with open(path, "rb") as f: dictionary = joblib.load(f) + meta = dictionary.pop("meta") name = dictionary.pop("name") config = Config(dictionary.pop("config")) cls_attributes = dictionary.pop("cls_attributes") - meta = dictionary.pop("meta") if name not in [c.__name__ for c in classes_with_traits(cls)]: raise TypeError( @@ -159,22 +149,53 @@ def read(cls, path, parent=None, subarray=None, **kwargs): f"one of its subclasses, but instead about {name}." ) - instance = Reconstructor.from_name( - name=name, - config=config, - **dictionary, - ) + if parent is not None: + if name in parent.config.keys(): + # Some configuration options should not be changed on a trained model. + forbidden_changes = [ + "model_cls", + "norm_cls", + "sign_cls", + "model_config", + "norm_config", + "sign_config", + "log_target", + "features", + ] + for trait_name in forbidden_changes: + if trait_name in parent.config[name].keys(): + raise ToolConfigurationError( + f"{name}.{trait_name} can not be changed when " + f"a {name} is loaded." + ) + + changed_traits = parent.config[name] + # add loaded config of reconstructor to current config + parent.config.update(config) + # re-add changes to config done when the tool is called + parent.config[name].update(changed_traits) + else: + parent.config.update(config) + + instance = Reconstructor.from_name( + name=name, + parent=parent, + subarray=dictionary["subarray"], + models=dictionary["models"], + ) + else: + instance = Reconstructor.from_name( + name=name, + config=config, + subarray=dictionary["subarray"], + models=dictionary["models"], + ) # set class attributes not handled by __init__, # e.g. the unit defined during SKLearnReconstructor.fit() for attr, value in cls_attributes.items(): setattr(instance, attr, value) - # first deal with kwargs that would need "special" treatment, parent and subarray - if parent is not None: - instance.parent = weakref.proxy(parent) - instance.log = parent.log.getChild(name) - if subarray is not None: if instance.subarray.telescope_types != subarray.telescope_types: instance.log.warning( @@ -182,9 +203,6 @@ def read(cls, path, parent=None, subarray=None, **kwargs): ) instance.subarray = subarray - for attr, value in kwargs.items(): - setattr(instance, attr, value) - Provenance().add_input_file(path, role="reconstructor", reference_meta=meta) return instance diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index 6ac8a85b518..bd81ba14bd3 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -118,6 +118,12 @@ class SKLearnReconstructor(Reconstructor): help="Which stereo combination method to use.", ).tag(config=True) + load_path = traits.Path( + default_value=None, + allow_none=True, + help="If given, load serialized model from this path.", + ).tag(config=True) + def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs): # Run the Component __init__ first to handle the configuration # and make `self.load_path` available @@ -634,6 +640,12 @@ class DispReconstructor(Reconstructor): help="Which stereo combination method to use.", ).tag(config=True) + load_path = traits.Path( + default_value=None, + allow_none=True, + help="If given, load serialized model from this path.", + ).tag(config=True) + def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs): # Run the Component __init__ first to handle the configuration # and make `self.load_path` available From abb17c6e14eb941f39e0b2c08393fceb9909a532 Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Thu, 12 Jun 2025 15:51:14 +0200 Subject: [PATCH 6/8] Re-add check for different subarray in inits --- src/ctapipe/reco/sklearn.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index bd81ba14bd3..b6476e46a25 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -161,7 +161,15 @@ def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs ) else: loaded = self.read(self.load_path) + if ( + subarray is not None + and loaded.subarray.telescope_types != subarray.telescope_types + ): + self.log.warning( + "Supplied subarray has different telescopes than subarray loaded from file" + ) self.__dict__.update(loaded.__dict__) + self.subarray = subarray if self.prefix is None: self.prefix = self.model_cls @@ -678,7 +686,15 @@ def __init__(self, subarray=None, atmosphere_profile=None, models=None, **kwargs ) else: loaded = self.read(self.load_path) + if ( + subarray is not None + and loaded.subarray.telescope_types != subarray.telescope_types + ): + self.log.warning( + "Supplied subarray has different telescopes than subarray loaded from file" + ) self.__dict__.update(loaded.__dict__) + self.subarray = subarray if self.prefix is None: self.prefix = "disp" From 072087d04d94c7e5bfe7365d4ef9237bd8dd419e Mon Sep 17 00:00:00 2001 From: Lukas Beiske <43672561+LukasBeiske@users.noreply.github.com> Date: Thu, 12 Jun 2025 16:10:42 +0200 Subject: [PATCH 7/8] No mutable default arguments Co-authored-by: Maximilian Linhoff --- src/ctapipe/reco/sklearn.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index b6476e46a25..96d73a993f5 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -421,7 +421,7 @@ def _predict_score(self, key, table): def _get_positive_index(self, key): return np.nonzero(self._models[key].classes_ == self.positive_class)[0][0] - def write(self, path, meta={}, overwrite=False): + def write(self, path, meta=None, overwrite=False): """ Save a dictionary using joblib-pickle, which contains all information/settings about an instance of a From eaf3d4f89f9327bbba816732cd3705bd199d661b Mon Sep 17 00:00:00 2001 From: Lukas Beiske Date: Thu, 12 Jun 2025 17:02:54 +0200 Subject: [PATCH 8/8] No mutable defaults --- src/ctapipe/reco/sklearn.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ctapipe/reco/sklearn.py b/src/ctapipe/reco/sklearn.py index 96d73a993f5..0e711ed1a44 100644 --- a/src/ctapipe/reco/sklearn.py +++ b/src/ctapipe/reco/sklearn.py @@ -300,7 +300,7 @@ def _table_to_y(self, table, mask=None): return np.log(y) return y - def write(self, path, meta={}, overwrite=False): + def write(self, path, meta=None, overwrite=False): """ Save a dictionary using joblib-pickle, which contains all information/settings about an instance of a @@ -928,7 +928,7 @@ def _set_n_jobs(self, n_jobs): disp.n_jobs = n_jobs.new sign.n_jobs = n_jobs.new - def write(self, path, meta={}, overwrite=False): + def write(self, path, meta=None, overwrite=False): """ Save a dictionary using joblib-pickle, which contains all information/settings about an instance of a ``DispReconstructor`` .