Misc petab.v2 fixes/cleanup (PEtab-dev#445)

* 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

Author: dweindl

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.

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,44 +1041,55 @@ 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
         observables: Include parameters from observableFormulas
         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())