add checks for mapping table

Author: m-philipps

petab/v2/lint.py

@@ -44,6 +44,7 @@
     "CheckPriorDistribution",
     "CheckUndefinedExperiments",
     "CheckInitialChangeSymbols",
+    "CheckMappingTable",
     "lint_problem",
     "default_validation_tasks",
 ]
@@ -551,7 +552,8 @@ def run(self, problem: Problem) -> ValidationIssue | None:
 
 class CheckAllParametersPresentInParameterTable(ValidationTask):
     """Ensure all required parameters are contained in the parameter table
-    with no additional ones."""
+    with no additional ones. This also ensures that the mapping table petab ids
+    are used in the PEtab problem."""
 
     def run(self, problem: Problem) -> ValidationIssue | None:
         if problem.model is None:
@@ -825,17 +827,17 @@ def run(self, problem: Problem) -> ValidationIssue | None:
 
             if parameter.prior_distribution not in PRIOR_DISTRIBUTIONS:
                 messages.append(
-                    f"Prior distribution `{parameter.prior_distribution}' "
-                    f"for parameter `{parameter.id}' is not valid."
+                    f"Prior distribution `{parameter.prior_distribution}` "
+                    f"for parameter `{parameter.id}` is not valid."
                 )
                 continue
 
             if (
                 exp_num_par := self._num_pars[parameter.prior_distribution]
             ) != len(parameter.prior_parameters):
                 messages.append(
-                    f"Prior distribution `{parameter.prior_distribution}' "
-                    f"for parameter `{parameter.id}' requires "
+                    f"Prior distribution `{parameter.prior_distribution}` "
+                    f"for parameter `{parameter.id}` requires "
                     f"{exp_num_par} parameters, but got "
                     f"{len(parameter.prior_parameters)} "
                     f"({parameter.prior_parameters})."
@@ -848,8 +850,8 @@ def run(self, problem: Problem) -> ValidationIssue | None:
                     _ = parameter.prior_dist.sample(1)
             except Exception as e:
                 messages.append(
-                    f"Prior parameters `{parameter.prior_parameters}' "
-                    f"for parameter `{parameter.id}' are invalid "
+                    f"Prior parameters `{parameter.prior_parameters}` "
+                    f"for parameter `{parameter.id}` are invalid "
                     f"(hint: {e})."
                 )
 
@@ -874,16 +876,16 @@ def run(self, problem: Problem) -> ValidationIssue | None:
                     continue
 
                 messages.append(
-                    f"Measurement `{measurement}' does not have a model ID, "
+                    f"Measurement `{measurement}` does not have a model ID, "
                     "but there are multiple models available. "
                     "Please specify the model ID in the measurement table."
                 )
                 continue
 
             if measurement.model_id not in available_models:
                 messages.append(
-                    f"Measurement `{measurement}' has model ID "
-                    f"`{measurement.model_id}' which does not match "
+                    f"Measurement `{measurement}` has model ID "
+                    f"`{measurement.model_id}` which does not match "
                     "any of the available models: "
                     f"{available_models}."
                 )
@@ -894,6 +896,102 @@ def run(self, problem: Problem) -> ValidationIssue | None:
         return None
 
 
+class CheckMappingTable(ValidationTask):
+    """Validate the mapping table."""
+
+    def run(self, problem: Problem) -> ValidationIssue | None:
+        messages = []
+
+        # Mapping table is optional
+        if problem.mappings:
+            # Check that each id only occurs once
+            counter = Counter(
+                [
+                    getattr(mapping, attr)
+                    for mapping in problem.mappings
+                    for attr in ["petab_id", "model_id"]
+                    if getattr(mapping, attr)
+                ]
+            )
+            non_unique = {id_ for id_, count in counter.items() if count > 1}
+            if non_unique:
+                return ValidationError(
+                    f"Mapping table contains non-unique IDs: {non_unique}."
+                )
+
+            # petabEntityId is not defined elsewhere in the PEtab problem
+            petab_ids_mapping = {m.petab_id for m in problem.mappings}
+            defined_petab_ids = (
+                {c.id for c in problem.conditions}
+                | {e.id for e in problem.experiments}
+                | {o.id for o in problem.observables}
+            )
+            if petab_ids_mapping & defined_petab_ids:
+                messages.append(
+                    f"PEtab IDs `{petab_ids_mapping & defined_petab_ids}` are "
+                    "defined in the mapping table but also defined through "
+                    "other PEtab tables."
+                )
+            # Grab all symbols from condition table for later
+            condition_target_ids_values = {
+                sym
+                for cond in problem.conditions
+                for change in cond.changes
+                for sym in (
+                    change.target_value.free_symbols
+                    | change.target_id.free_symbols
+                )
+            }
+
+            for mapping in problem.mappings:
+                # petabEntityId is not referenced in any model
+                for model in problem.models:
+                    if model.has_entity_with_id(mapping.petab_id):
+                        messages.append(
+                            f"`{mapping.petab_id}` is used in the mapping "
+                            "table and referenced directly in the model "
+                            f"`{model.model_id}`."
+                        )
+
+                # modelEntityId can be empty
+                if mapping.model_id:
+                    # model_id is not in petab problem
+                    if mapping.model_id in defined_petab_ids:
+                        messages.append(
+                            f"PEtab ID `{mapping.model_id}` mirrors the "
+                            "model entity ID referenced in the mapping table."
+                        )
+                    if mapping.model_id in condition_target_ids_values:
+                        messages.append(
+                            f"Identifier `{mapping.model_id}` is mapped to a "
+                            "PEtab ID in the mapping table, but also directly "
+                            "used in the conditions table."
+                        )
+                    for observable_formula in [
+                        o.formula for o in problem.observables
+                    ]:
+                        if mapping.model_id in observable_formula.free_symbols:
+                            messages.append(
+                                f"Identifier `{mapping.model_id}` is mapped "
+                                "to a PEtab ID in the mapping table, but also "
+                                "directly used in an observable formula."
+                            )
+                    for obs_noise_formula in [
+                        o.noise_formula for o in problem.observables
+                    ]:
+                        if mapping.model_id in obs_noise_formula.free_symbols:
+                            messages.append(
+                                f"Identifier `{mapping.model_id}` is mapped "
+                                "to a PEtab ID in the mapping table, but also "
+                                "directly used in a noise formula."
+                            )
+
+        if messages:
+            return ValidationError("\n".join(messages))
+
+        return None
+
+
 def get_valid_parameters_for_parameter_table(
     problem: Problem,
 ) -> set[str]:
@@ -933,9 +1031,13 @@ def get_valid_parameters_for_parameter_table(
         if p not in invalid
     )
 
+    # Add petab ids from mapping table if they are used for aliasing
     for mapping in problem.mappings:
-        if mapping.model_id and mapping.model_id in parameter_ids.keys():
+        if mapping.model_id:
             parameter_ids[mapping.petab_id] = None
+            # An aliased model id is not a valid parameter id
+            if mapping.model_id in parameter_ids:
+                del parameter_ids[mapping.model_id]
 
     # add output parameters from observable table
     output_parameters = problem.get_output_parameters()
@@ -977,20 +1079,13 @@ def get_required_parameters_for_parameter_table(
         measurement table as well as all parametric condition table overrides
         that are not defined in the model.
     """
-    parameter_ids = set()
-    condition_targets = {
-        change.target_id
-        for cond in problem.conditions
-        for change in cond.changes
-    }
+    # Start with mapping table petab ids
+    parameter_ids = {m.petab_id for m in problem.mappings}
 
     # Add parameters from measurement table, unless they are fixed parameters
     def append_overrides(overrides):
         parameter_ids.update(
-            str_p
-            for p in overrides
-            if isinstance(p, sp.Symbol)
-            and (str_p := str(p)) not in condition_targets
+            str(p) for p in overrides if isinstance(p, sp.Symbol)
         )
 
     for m in problem.measurements:
@@ -1033,7 +1128,12 @@ def append_overrides(overrides):
         if not problem.model.has_entity_with_id(str(p))
     )
 
-    # parameters that are overridden via the condition table are not allowed
+    # Parameters that are overridden via the condition table are not allowed
+    condition_targets = {
+        change.target_id
+        for cond in problem.conditions
+        for change in cond.changes
+    }
     parameter_ids -= condition_targets
 
     return parameter_ids
@@ -1090,5 +1190,5 @@ def get_placeholders(
     CheckUnusedConditions(),
     CheckPriorDistribution(),
     CheckInitialChangeSymbols(),
-    # TODO validate mapping table
+    CheckMappingTable(),
 ]

tests/v2/test_lint.py

@@ -2,8 +2,11 @@
 
 from copy import deepcopy
 
+from petabtests import DEFAULT_PYSB_FILE
+
 from petab.v2 import Problem
 from petab.v2.lint import *
+from petab.v2.models.pysb_model import PySBModel
 from petab.v2.models.sbml_model import SbmlModel
 
 
@@ -43,7 +46,7 @@ def test_invalid_model_id_in_measurements():
     """Test that measurements with an invalid model ID are caught."""
     problem = Problem()
     problem.models.append(SbmlModel.from_antimony("p1 = 1", model_id="model1"))
-    problem.add_observable("obs1", "A")
+    problem.add_observable("obs1", "A", 1)
     problem.add_measurement("obs1", experiment_id="e1", time=0, measurement=1)
 
     check = CheckMeasurementModelId()
@@ -70,7 +73,7 @@ def test_undefined_experiment_id_in_measurements():
     """Test that measurements with an undefined experiment ID are caught."""
     problem = Problem()
     problem.add_experiment("e1", 0, "c1")
-    problem.add_observable("obs1", "A")
+    problem.add_observable("obs1", "A", 1)
     problem.add_measurement("obs1", experiment_id="e1", time=0, measurement=1)
 
     check = CheckUndefinedExperiments()
@@ -107,3 +110,48 @@ def test_validate_initial_change_symbols():
     problem.parameter_tables[0].parameters.remove(problem["p2"])
     assert (error := check.run(problem)) is not None
     assert "contains additional symbols: {'p2'}" in error.message
+
+
+def test_check_mapping_table():
+    """Test checks related to the mapping table."""
+    problem = Problem()
+    # PySB model from PEtab test suite
+    problem.model = PySBModel.from_file(DEFAULT_PYSB_FILE)
+    problem.add_mapping(
+        petab_id="A_comp",
+        model_id="A_() ** compartment",
+        name=None,
+    )
+    problem.add_parameter(
+        "A_comp",
+        estimate=True,
+        nominal_value=2,
+        lb=0,
+        ub=10,
+    )
+
+    check = CheckMappingTable()
+    assert check.run(problem) is None
+
+    check = CheckAllParametersPresentInParameterTable()
+    assert check.run(problem) is None
+
+    # add a petab id without model id but with name for annotation
+    problem.add_mapping(petab_id="p2", model_id=None, name="Parameter 2")
+    problem.add_parameter("p2", estimate=True, nominal_value=1, lb=0, ub=10)
+
+    check = CheckMappingTable()
+    assert check.run(problem) is None
+
+    # Invalid: petabEntityId is referenced in the model
+    problem.mapping_tables = []
+    problem.add_mapping(
+        petab_id="a0",
+        model_id="A_() ** compartment",
+        name=None,
+    )
+    assert (error := check.run(problem)) is not None
+    assert (
+        "`a0` is used in the mapping table and referenced directly"
+        in error.message
+    )