From dc722a7f72a43fe67a94eaeb3e02eced2cd29b96 Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Thu, 2 Oct 2025 15:04:09 +0200 Subject: [PATCH] Misc petab.v2 fixes/cleanup * Remove `Problem.from_dfs` which isn't really useful * Resolve some TODOs * Fix doctests * Fix `get_output_parameters` to not include observable IDs and make more efficient --- petab/v2/core.py | 82 +++++++++++++----------------------------------- petab/v2/lint.py | 72 ++++++++++++++++++++++-------------------- 2 files changed, 59 insertions(+), 95 deletions(-) diff --git a/petab/v2/core.py b/petab/v2/core.py index b331c713..6d117f92 100644 --- a/petab/v2/core.py +++ b/petab/v2/core.py @@ -992,16 +992,13 @@ def _validate(self) -> Self: "Estimated parameter must have lower and upper bounds set" ) - # TODO: also if not estimated? - if ( - self.estimate - and self.lb is not None - and self.ub is not None - and self.lb >= self.ub - ): - raise ValueError("Lower bound must be less than upper bound.") + if self.lb is not None and self.ub is not None and self.lb > self.ub: + raise ValueError( + "Lower bound must be less than or equal to upper bound." + ) - # TODO priorType, priorParameters + # NOTE: priorType and priorParameters are currently checked in + # `CheckPriorDistribution` return self @@ -1294,50 +1291,6 @@ def from_yaml( mapping_tables=mapping_tables, ) - @staticmethod - def from_dfs( - model: Model = None, - condition_df: pd.DataFrame = None, - experiment_df: pd.DataFrame = None, - measurement_df: pd.DataFrame = None, - parameter_df: pd.DataFrame = None, - observable_df: pd.DataFrame = None, - mapping_df: pd.DataFrame = None, - config: ProblemConfig = None, - ): - """ - Construct a PEtab problem from dataframes. - - Parameters: - condition_df: PEtab condition table - experiment_df: PEtab experiment table - measurement_df: PEtab measurement table - parameter_df: PEtab parameter table - observable_df: PEtab observable table - mapping_df: PEtab mapping table - model: The underlying model - config: The PEtab problem configuration - """ - # TODO: do we really need this? - - observable_table = ObservableTable.from_df(observable_df) - condition_table = ConditionTable.from_df(condition_df) - experiment_table = ExperimentTable.from_df(experiment_df) - measurement_table = MeasurementTable.from_df(measurement_df) - mapping_table = MappingTable.from_df(mapping_df) - parameter_table = ParameterTable.from_df(parameter_df) - - return Problem( - models=[model], - condition_tables=[condition_table], - experiment_tables=[experiment_table], - observable_tables=[observable_table], - measurement_tables=[measurement_table], - parameter_tables=[parameter_table], - mapping_tables=[mapping_table], - config=config, - ) - @staticmethod def from_combine(filename: Path | str) -> Problem: """Read PEtab COMBINE archive (http://co.mbine.org/documents/archive). @@ -2235,6 +2188,7 @@ def model_dump(self, **kwargs) -> dict[str, Any]: 'experiment_files': [], 'extensions': {}, 'format_version': '2.0.0', + 'id': None, 'mapping_files': [], 'measurement_files': [], 'model_files': {}, @@ -2343,19 +2297,25 @@ class ProblemConfig(BaseModel): #: The problem ID. id: str | None = None - #: The path to the parameter file, relative to ``base_path``. - # TODO https://github.com/PEtab-dev/PEtab/pull/641: - # rename to parameter_files in yaml for consistency with other files? - # always a list? - parameter_files: list[AnyUrl | Path] = Field( - default=[], alias=C.PARAMETER_FILES - ) - + #: The paths to the parameter tables. + # Absolute or relative to `base_path`. + parameter_files: list[AnyUrl | Path] = [] + #: The model IDs and files used by the problem (`id->ModelFile`). model_files: dict[str, ModelFile] | None = {} + #: The paths to the measurement tables. + # Absolute or relative to `base_path`. measurement_files: list[AnyUrl | Path] = [] + #: The paths to the condition tables. + # Absolute or relative to `base_path`. condition_files: list[AnyUrl | Path] = [] + #: The paths to the experiment tables. + # Absolute or relative to `base_path`. experiment_files: list[AnyUrl | Path] = [] + #: The paths to the observable tables. + # Absolute or relative to `base_path`. observable_files: list[AnyUrl | Path] = [] + #: The paths to the mapping tables. + # Absolute or relative to `base_path`. mapping_files: list[AnyUrl | Path] = [] #: Extensions used by the problem. diff --git a/petab/v2/lint.py b/petab/v2/lint.py index 4d864b57..20f5dfc1 100644 --- a/petab/v2/lint.py +++ b/petab/v2/lint.py @@ -992,11 +992,6 @@ def append_overrides(overrides): append_overrides(m.observable_parameters) append_overrides(m.noise_parameters) - # TODO remove `observable_ids` when - # `get_output_parameters` is updated for PEtab v2/v1.1, where - # observable IDs are allowed in observable formulae - observable_ids = {o.id for o in problem.observables} - # Add output parameters except for placeholders for formula_type, placeholder_sources in ( ( @@ -1021,9 +1016,7 @@ def append_overrides(overrides): **placeholder_sources, ) parameter_ids.update( - p - for p in output_parameters - if p not in placeholders and p not in observable_ids + p for p in output_parameters if p not in placeholders ) # Add condition table parametric overrides unless already defined in the @@ -1048,8 +1041,8 @@ def get_output_parameters( ) -> list[str]: """Get output parameters - Returns IDs of parameters used in observable and noise formulas that are - not defined in the model. + Returns IDs of symbols used in observable and noise formulas that are + not observables and that are not defined in the model. Arguments: problem: The PEtab problem @@ -1057,35 +1050,46 @@ def get_output_parameters( noise: Include parameters from noiseFormulas Returns: - List of output parameter IDs + List of output parameter IDs, including any placeholder parameters. """ - formulas = [] + # collect free symbols from observable and noise formulas, + # skipping observable IDs + candidates = set() if observables: - formulas.extend(o.formula for o in problem.observables) + candidates |= { + str_sym + for o in problem.observables + if o.formula is not None + for sym in o.formula.free_symbols + if (str_sym := str(sym)) != o.id + } if noise: - formulas.extend(o.noise_formula for o in problem.observables) - output_parameters = OrderedDict() + candidates |= { + str_sym + for o in problem.observables + if o.noise_formula is not None + for sym in o.noise_formula.free_symbols + if (str_sym := str(sym)) != o.id + } - for formula in formulas: - free_syms = sorted( - formula.free_symbols, - key=lambda symbol: symbol.name, - ) - for free_sym in free_syms: - sym = str(free_sym) - if problem.model.symbol_allowed_in_observable_formula(sym): - continue + output_parameters = OrderedDict() - # does it map to a model entity? - for mapping in problem.mappings: - if mapping.petab_id == sym and mapping.model_id is not None: - if problem.model.symbol_allowed_in_observable_formula( - mapping.model_id - ): - break - else: - # no mapping to a model entity, so it is an output parameter - output_parameters[sym] = None + # filter out symbols that are defined in the model or mapped to + # such symbols + for candidate in sorted(candidates): + if problem.model.symbol_allowed_in_observable_formula(candidate): + continue + + # does it map to a model entity? + for mapping in problem.mappings: + if mapping.petab_id == candidate and mapping.model_id is not None: + if problem.model.symbol_allowed_in_observable_formula( + mapping.model_id + ): + break + else: + # no mapping to a model entity, so it is an output parameter + output_parameters[candidate] = None return list(output_parameters.keys())