From 5d8faace30faff8f80b24edab976b48a5009101c Mon Sep 17 00:00:00 2001 From: dilpath Date: Fri, 29 Nov 2024 17:38:14 +0100 Subject: [PATCH] fix tests --- petab_select/candidate_space.py | 6 +- petab_select/models.py | 363 +++++++++++++++++++------------- petab_select/ui.py | 23 ++ test/pypesto/test_pypesto.py | 3 +- test/ui/test_ui.py | 3 +- 5 files changed, 254 insertions(+), 144 deletions(-) diff --git a/petab_select/candidate_space.py b/petab_select/candidate_space.py index fad615e6..b559bbc8 100644 --- a/petab_select/candidate_space.py +++ b/petab_select/candidate_space.py @@ -155,7 +155,11 @@ def set_iteration_user_calibrated_models( iteration_user_calibrated_models = Models() for model in self.models: if ( - (user_model := user_calibrated_models[model.get_hash()]) + ( + user_model := user_calibrated_models.get( + model.get_hash(), None + ) + ) is not None ) and ( user_model.get_criterion( diff --git a/petab_select/models.py b/petab_select/models.py index f712add2..49359b45 100644 --- a/petab_select/models.py +++ b/petab_select/models.py @@ -28,119 +28,38 @@ ModelIndex: TypeAlias = int | ModelHash | slice | str | Iterable __all__ = [ + "ListDict", "Models", "models_from_yaml_list", "models_to_yaml_list", ] -class Models(MutableSequence): - """A collection of models. - - Behaves like a list of models, but also supports operations - involving objects that can be mapped to model(s). For example, model hashes - can be used to add or access models. - - Some list methods are not yet implemented -- feel free to request anything - that feels intuitive. - - Provide a PEtab Select ``problem`` to the constructor or via - ``set_problem``, to use add models by hashes. This means that all models - must belong to the same PEtab Select problem. - """ - - def set_problem(self, problem: Problem) -> None: - """Set the PEtab Select problem for this set of models.""" - self._problem = problem - - def lint(self): - """Lint the models, e.g. check all hashes are unique. +class ListDict(MutableSequence): + """Acts like a ``list`` and a ``dict``. - Currently raises an exception when invalid. - """ - duplicates = [ - model_hash - for model_hash, count in Counter(self._hashes).items() - if count > 1 - ] - if duplicates: - raise ValueError( - "Multiple models exist with the same hash. " - f"Model hashes: `{duplicates}`." - ) - - @staticmethod - def from_yaml( - models_yaml: TYPE_PATH, - petab_problem: petab.Problem = None, - problem: Problem = None, - ) -> Models: - """Generate models from a PEtab Select list of model YAML file. - - Args: - models_yaml: - The path to the PEtab Select list of model YAML file. - petab_problem: - See :meth:`Model.from_dict`. - problem: - The PEtab Select problem. - - Returns: - The models. - """ - with open(str(models_yaml)) as f: - model_dict_list = yaml.safe_load(f) - if not model_dict_list: - # Empty file - models = [] - elif not isinstance(model_dict_list, list): - # File contains a single model - models = [ - Model.from_dict( - model_dict_list, - base_path=Path(models_yaml).parent, - petab_problem=petab_problem, - ) - ] - else: - # File contains a list of models - models = [ - Model.from_dict( - model_dict, - base_path=Path(models_yaml).parent, - petab_problem=petab_problem, - ) - for model_dict in model_dict_list - ] + Not all methods are implemented -- feel free to request anything that you + think makes sense for a ``list`` or ``dict`` object. - return Models(models=models, problem=problem) + The context is a list of objects that may have some metadata (e.g. a hash) + associated with each of them. The objects can be operated on like a list, + or requested like a dict, by their metadata (hash). - def to_yaml( - self, - output_yaml: TYPE_PATH, - relative_paths: bool = True, - ) -> None: - """Generate a YAML listing of models. + Mostly based on ``UserList`` and ``UserDict``, but some methods are + currently not yet implemented. + https://github.com/python/cpython/blob/main/Lib/collections/__init__.py - Args: - output_yaml: - The location where the YAML will be saved. - relative_paths: - Whether to rewrite the paths in each model (e.g. the path to the - model's PEtab problem) relative to the `output_yaml` location. - """ - paths_relative_to = None - if relative_paths: - paths_relative_to = Path(output_yaml).parent - model_dicts = [ - model.to_dict(paths_relative_to=paths_relative_to) - for model in self - ] - with open(output_yaml, "w") as f: - yaml.safe_dump(model_dicts, f) + The typing is currently based on PEtab Select objects. Hence, objects are + in ``_models``, and metadata (model hashes) are in ``_hashes``. - # `list` methods. Compared to `UserList`, some methods are skipped. - # https://github.com/python/cpython/blob/main/Lib/collections/__init__.py + Attributes: + _models: + The list of objects (list items/dictionary values) + (PEtab Select models). + _hashes: + The list of metadata (dictionary keys) (model hashes). + _problem: + """ def __init__( self, models: Iterable[ModelLike] = None, problem: Problem = None @@ -184,27 +103,54 @@ def __len__(self) -> int: return len(self._models) def __getitem__( - self, item: ModelIndex | Iterable[ModelIndex] + self, key: ModelIndex | Iterable[ModelIndex] ) -> Model | Models: - match item: - case int(): - return self._models[item] + try: + match key: + case int(): + return self._models[key] + case ModelHash() | str(): + return self._models[self._hashes.index(key)] + case slice(): + print(key) + return self.__class__(self._models[key]) + case Iterable(): + # TODO sensible to yield here? + return [self[key_] for key_ in key] + case _: + raise TypeError(f"Unexpected type: `{type(key)}`.") + except ValueError as err: + raise KeyError from err + + def _model_like_to_model(self, model_like: ModelLike) -> Model: + """Get the model that corresponds to a model-like object. + + Args: + model_like: + Something that uniquely identifies a model; a model or a model + hash. + + Returns: + The model. + """ + match model_like: + case Model(): + model = model_like case ModelHash() | str(): - return self._models[self._hashes.index(item)] - case slice(): - return self.__class__(self._models[item]) - case Iterable(): - # TODO sensible to yield here? - return [self[item_] for item_ in item] + model = self._problem.model_hash_to_model(model_like) case _: - raise TypeError(f"Unexpected type: `{type(item)}`.") + raise TypeError(f"Unexpected type: `{type(model_like)}`.") + return model def __setitem__(self, key: ModelIndex, item: ModelLike) -> None: match key: case int(): pass case ModelHash() | str(): - key = self._hashes.index(key) + if key in self._hashes: + key = self._hashes.index(key) + else: + key = len(self) case slice(): for key_, item_ in zip( range(*key.indices(len(self))), item, strict=True @@ -216,15 +162,9 @@ def __setitem__(self, key: ModelIndex, item: ModelLike) -> None: case _: raise TypeError(f"Unexpected type: `{type(key)}`.") - match item: - case Model(): - pass - case ModelHash() | str(): - item = self._problem.model_hash_to_model(item) - case _: - raise TypeError(f"Unexpected type: `{type(item)}`.") + item = self._model_like_to_model(model_like=item) - if key < len(self._models): + if key < len(self): self._models[key] = item self._hashes[key] = item.get_hash() else: @@ -235,18 +175,49 @@ def __setitem__(self, key: ModelIndex, item: ModelLike) -> None: # to add a new model. self.append(item) + def _update(self, index: int, item: ModelLike) -> None: + """Update the models by adding a new model or overwriting an old model. + + Args: + index: + The index where the model will be inserted, if it doesn't + already exist. + item: + A model or a model hash. + """ + model = self._model_like_to_model(item) + if model.get_hash() in self: + warnings.warn( + ( + f"A model with hash `{model.get_hash()}` already exists " + "in this collection of models. The previous model will be " + "overwritten." + ), + RuntimeWarning, + stacklevel=2, + ) + self[model.get_hash()] = model + else: + self._models.insert(index, None) + self._hashes.insert(index, None) + # Re-use __setitem__ logic + self[index] = item + def __delitem__(self, key: ModelIndex) -> None: - match key: - case ModelHash() | str(): - key = self._hashes.index(key) - case slice(): - for key_ in range(*key.indices(len(self))): - del self[key_] - case Iterable(): - for key_ in key: - del self[key_] - case _: - raise TypeError(f"Unexpected type: `{type(key)}`.") + try: + match key: + case ModelHash() | str(): + key = self._hashes.index(key) + case slice(): + for key_ in range(*key.indices(len(self))): + del self[key_] + case Iterable(): + for key_ in key: + del self[key_] + case _: + raise TypeError(f"Unexpected type: `{type(key)}`.") + except ValueError as err: + raise KeyError from err del self._models[key] del self._hashes[key] @@ -285,16 +256,10 @@ def __copy__(self) -> Models: return Models(models=self._models, problem=self._problem) def append(self, item: ModelLike) -> None: - # Re-use __setitem__ logic - self._models.append(None) - self._hashes.append(None) - self[-1] = item + self._update(index=len(self), item=item) def insert(self, index: int, item: ModelLike): - # Re-use __setitem__ logic - self._models.insert(index, None) - self._hashes.insert(index, None) - self[index] = item + self._update(index=len(self), item=item) # def pop(self, index: int = -1): # model = self._models[index] @@ -324,6 +289,122 @@ def extend(self, other: Iterable[ModelLike]) -> None: for model_like in other: self.append(model_like) + # __iter__/__next__? Not in UserList... + + # `dict` methods. + + def get( + self, + key: ModelIndex | Iterable[ModelIndex], + default: ModelLike | None = None, + ) -> Model | Models: + try: + return self[key] + except KeyError: + return default + + +class Models(ListDict): + """A collection of models. + + Provide a PEtab Select ``problem`` to the constructor or via + ``set_problem``, to use add models by hashes. This means that all models + must belong to the same PEtab Select problem. + + This permits both ``list`` and ``dict`` operations -- see + :class:``ListDict`` for further details. + """ + + def set_problem(self, problem: Problem) -> None: + """Set the PEtab Select problem for this set of models.""" + self._problem = problem + + def lint(self): + """Lint the models, e.g. check all hashes are unique. + + Currently raises an exception when invalid. + """ + duplicates = [ + model_hash + for model_hash, count in Counter(self._hashes).items() + if count > 1 + ] + if duplicates: + raise ValueError( + "Multiple models exist with the same hash. " + f"Model hashes: `{duplicates}`." + ) + + @staticmethod + def from_yaml( + models_yaml: TYPE_PATH, + petab_problem: petab.Problem = None, + problem: Problem = None, + ) -> Models: + """Generate models from a PEtab Select list of model YAML file. + + Args: + models_yaml: + The path to the PEtab Select list of model YAML file. + petab_problem: + See :meth:`Model.from_dict`. + problem: + The PEtab Select problem. + + Returns: + The models. + """ + with open(str(models_yaml)) as f: + model_dict_list = yaml.safe_load(f) + if not model_dict_list: + # Empty file + models = [] + elif not isinstance(model_dict_list, list): + # File contains a single model + models = [ + Model.from_dict( + model_dict_list, + base_path=Path(models_yaml).parent, + petab_problem=petab_problem, + ) + ] + else: + # File contains a list of models + models = [ + Model.from_dict( + model_dict, + base_path=Path(models_yaml).parent, + petab_problem=petab_problem, + ) + for model_dict in model_dict_list + ] + + return Models(models=models, problem=problem) + + def to_yaml( + self, + output_yaml: TYPE_PATH, + relative_paths: bool = True, + ) -> None: + """Generate a YAML listing of models. + + Args: + output_yaml: + The location where the YAML will be saved. + relative_paths: + Whether to rewrite the paths in each model (e.g. the path to the + model's PEtab problem) relative to the `output_yaml` location. + """ + paths_relative_to = None + if relative_paths: + paths_relative_to = Path(output_yaml).parent + model_dicts = [ + model.to_dict(paths_relative_to=paths_relative_to) + for model in self + ] + with open(output_yaml, "w") as f: + yaml.safe_dump(model_dicts, f) + def models_from_yaml_list( model_list_yaml: TYPE_PATH, diff --git a/petab_select/ui.py b/petab_select/ui.py index d2dd3f1a..b06373f8 100644 --- a/petab_select/ui.py +++ b/petab_select/ui.py @@ -1,4 +1,5 @@ import copy +import warnings from pathlib import Path from typing import Any @@ -93,6 +94,17 @@ def start_iteration( - add `Iteration` class to manage an iteration, append to `CandidateSpace.iterations`? """ + if isinstance(user_calibrated_models, dict): + warnings.warn( + ( + "`calibrated_models` should be a `petab_select.Models` object." + "e.g. `calibrated_models = " + "petab_select.Models(old_calibrated_models.values())`." + ), + DeprecationWarning, + stacklevel=2, + ) + user_calibrated_models = Models(user_calibrated_models.values()) do_search = True # FIXME might be difficult for a CLI tool to specify a specific predecessor # model if their candidate space has models. Need a way to empty @@ -239,6 +251,17 @@ def end_iteration( Whether PEtab Select has decided to end the model selection, as a boolean. """ + if isinstance(calibrated_models, dict): + warnings.warn( + ( + "`calibrated_models` should be a `petab_select.Models` object." + "e.g. `calibrated_models = " + "petab_select.Models(old_calibrated_models.values())`." + ), + DeprecationWarning, + stacklevel=2, + ) + calibrated_models = Models(calibrated_models.values()) iteration_results = { MODELS: candidate_space.get_iteration_calibrated_models( calibrated_models=calibrated_models, diff --git a/test/pypesto/test_pypesto.py b/test/pypesto/test_pypesto.py index de6f2576..e670d8cc 100644 --- a/test/pypesto/test_pypesto.py +++ b/test/pypesto/test_pypesto.py @@ -19,6 +19,7 @@ # Set to `[]` to test all test_cases = [ + # '0001', # '0006', # '0002', # '0008', @@ -77,7 +78,7 @@ def test_pypesto(test_case_path_stem): # Get the best model best_model = petab_select_problem.get_best( - models=pypesto_select_problem.calibrated_models.values(), + models=pypesto_select_problem.calibrated_models, ) # Load the expected model. diff --git a/test/ui/test_ui.py b/test/ui/test_ui.py index dcfaac7f..7efb442b 100644 --- a/test/ui/test_ui.py +++ b/test/ui/test_ui.py @@ -4,6 +4,7 @@ from more_itertools import one import petab_select +from petab_select import Models from petab_select.constants import ( CANDIDATE_SPACE, MODELS, @@ -30,7 +31,7 @@ def test_user_calibrated_models(petab_select_problem): model_M1_2.set_criterion( criterion=petab_select_problem.criterion, value=12.3 ) - user_calibrated_models = {model_M1_2.get_hash(): model_M1_2} + user_calibrated_models = Models([model_M1_2]) # Initial iteration: expect the "empty" model. Set dummy criterion and continue. iteration = petab_select.ui.start_iteration(