Merge branch 'lint_mapping_table' into sciml_lint

Author: dilpath

petab/v2/lint.py

@@ -447,31 +447,31 @@ def run(self, problem: Problem) -> ValidationIssue | None:
 
         # check for uniqueness of all primary keys
         counter = Counter(c.id for c in problem.conditions)
-        duplicates = {id_ for id_, count in counter.items() if count > 1}
+        duplicates = sorted(id_ for id_, count in counter.items() if count > 1)
 
         if duplicates:
             return ValidationError(
                 f"Condition table contains duplicate IDs: {duplicates}"
             )
 
         counter = Counter(o.id for o in problem.observables)
-        duplicates = {id_ for id_, count in counter.items() if count > 1}
+        duplicates = sorted(id_ for id_, count in counter.items() if count > 1)
 
         if duplicates:
             return ValidationError(
                 f"Observable table contains duplicate IDs: {duplicates}"
             )
 
         counter = Counter(e.id for e in problem.experiments)
-        duplicates = {id_ for id_, count in counter.items() if count > 1}
+        duplicates = sorted(id_ for id_, count in counter.items() if count > 1)
 
         if duplicates:
             return ValidationError(
                 f"Experiment table contains duplicate IDs: {duplicates}"
             )
 
         counter = Counter(p.id for p in problem.parameters)
-        duplicates = {id_ for id_, count in counter.items() if count > 1}
+        duplicates = sorted(id_ for id_, count in counter.items() if count > 1)
 
         if duplicates:
             return ValidationError(
@@ -510,7 +510,11 @@ def run(self, problem: Problem) -> ValidationIssue | None:
         for experiment in problem.experiments:
             # Check that there are no duplicate timepoints
             counter = Counter(period.time for period in experiment.periods)
-            duplicates = {time for time, count in counter.items() if count > 1}
+            duplicates = sorted(
+                time
+                for time, count in counter.items()
+                if count > 1
+            )
             if duplicates:
                 messages.append(
                     f"Experiment {experiment.id} contains duplicate "
@@ -905,38 +909,47 @@ def run(self, problem: Problem) -> ValidationIssue | None:
 
         # Mapping table is optional
         if problem.mappings:
-            # Check that each id only occurs once
-            counter = Counter(
+            # Check that each id, across both the petabEntityId and
+            # modelEntityId columns, occurs only once
+            must_be_unique_ids = []
+            for mapping in problem.mappings:
+                petab_id := getattr(mapping, "petab_id"):
+                model_id := getattr(mapping, "model_id"):
+
+                if petab_id:
+                    must_be_unique_ids.append(petab_id)
+                # Duplicates for annotation-only rows (identity mappings)
+                # are permitted.
+                if petab_id == model_id:
+                    continue
+                if model_id:
+                    must_be_unique_ids.append(model_id)
+
+            non_unique_ids = sorted(
                 id_
-                for mapping in problem.mappings
-                for id_ in {
-                    getattr(mapping, attr)
-                    for attr in ["petab_id", "model_id"]
-                    if getattr(mapping, attr)
-                }
-            )
-            non_unique = sorted(
-                {id_ for id_, count in counter.items() if count > 1}
+                for id_, count in Counter(must_be_unique_ids).items()
+                if count > 1
             )
-            if non_unique:
+            if non_unique_ids:
                 return ValidationError(
-                    f"Mapping table contains non-unique IDs: {non_unique}."
+                    f"Mapping table contains non-unique IDs: {non_unique_ids}."
                 )
 
             # petabEntityId is not defined elsewhere in the PEtab problem
-            petab_ids_mapping = {
+            new_petab_ids = {
                 m.petab_id
                 for m in problem.mappings
+                # Ignore identity mappings used for annotation
                 if m.petab_id != m.model_id
             }
-            defined_petab_ids = (
+            old_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:
+            if overdefined_ids := sorted(new_petab_ids & old_petab_ids):
                 messages.append(
-                    f"PEtab IDs `{petab_ids_mapping & defined_petab_ids}` are "
+                    f"PEtab IDs `{overdefined_ids}` are "
                     "defined in the mapping table but also defined through "
                     "other PEtab tables."
                 )
@@ -1040,6 +1053,9 @@ def get_valid_parameters_for_parameter_table(
     )
 
     # Add petab ids from mapping table if they are used for aliasing
+    # FIXME only add mapping.petab_id to allowed parameter IDs list if it
+    #       aliases an invalid PEtab ID? See
+    #       https://github.com/PEtab-dev/libpetab-python/pull/482#discussion_r3420762034
     for mapping in problem.mappings:
         if mapping.petab_id not in invalid:
             parameter_ids[mapping.petab_id] = None

tests/v2/test_lint.py

@@ -112,7 +112,7 @@ def test_validate_initial_change_symbols():
 def test_check_mapping_table():
     """Test checks related to the mapping table."""
     problem = Problem()
-    # SBML model with mappings
+    # FIXME see https://github.com/PEtab-dev/libpetab-python/pull/482#discussion_r3431330125
     problem.model = SbmlModel.from_antimony("a.mean = 1")
     problem.add_mapping(
         petab_id="a_m",