diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e197d6..e6c94f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ Milestones in the raven-toolbox port. For function-level status see [docs/raven_migration.md](https://github.com/SysBioChalmers/raven-toolbox/blob/develop/docs/reference/migration.md); for open work see [docs/todo.md](https://github.com/SysBioChalmers/raven-toolbox/blob/develop/docs/reference/todo.md). +## Unreleased + +* **`diff_models` compares grRules as logic, not text.** The GPR check now DNF-expands each rule (via the + existing `manipulation.gpr_to_dnf`), sorts the genes within each isozyme clause and sorts the clauses, so + operand order no longer registers as a difference: `a and b` == `b and a` and `a or b` == `b or a`. The + previous heuristic only lowercased and collapsed whitespace, so it flagged logically identical rules that + differed only in operand order. This brings `diff_models` in line with MATLAB RAVEN's `diffModels` + ([RAVEN #686](https://github.com/SysBioChalmers/RAVEN/pull/686)); a rule cobra cannot parse falls back to + the old string comparison, so malformed rules are still compared rather than silently equated. + ## 0.3.0 — 2026-07-16 Compartment localisation and per-reaction confidence tracking, new gap-filling and flux-sampling diff --git a/src/raven_toolbox/comparison/diff.py b/src/raven_toolbox/comparison/diff.py index 44dee75..e432688 100644 --- a/src/raven_toolbox/comparison/diff.py +++ b/src/raven_toolbox/comparison/diff.py @@ -10,8 +10,8 @@ Diff scope: reaction / metabolite / gene id sets, stoichiometry (within tolerance), bounds, objective coefficients, GPR rules, metabolite formula/charge/compartment, and a configurable set of annotation keys. -Formatting differences (key ordering, whitespace, float repr) are -explicitly **not** failures. +Formatting differences (key ordering, whitespace, float repr, and GPR +operand order — ``a and b`` == ``b and a``) are explicitly **not** failures. """ from __future__ import annotations @@ -20,6 +20,8 @@ import cobra +from raven_toolbox.manipulation.expand import gpr_to_dnf + # Annotation keys checked by default. Add via ``extra_annotations`` or # remove via ``ignore_annotations`` in :func:`diff_models`. DEFAULT_ANNOTATION_KEYS: tuple[str, ...] = ( @@ -152,10 +154,9 @@ def _diff_reactions( f"{rxn_id}: objective A={ra.objective_coefficient} B={rb.objective_coefficient}", ) - ga = _normalise_gpr(ra.gene_reaction_rule) - gb = _normalise_gpr(rb.gene_reaction_rule) - if ga != gb: - _push(diffs, counters, "gpr", cap, f"{rxn_id}: GPR A={ga!r} B={gb!r}") + if _canonical_gpr(ra) != _canonical_gpr(rb): + _push(diffs, counters, "gpr", cap, + f"{rxn_id}: GPR A={ra.gene_reaction_rule!r} B={rb.gene_reaction_rule!r}") _diff_annotations( f"rxn {rxn_id}", ra.annotation, rb.annotation, @@ -218,16 +219,30 @@ def _diff_annotations( _push(diffs, counters, "anno", cap, f"{label}.annotation[{k!r}]: A={va} B={vb}") -def _normalise_gpr(rule: str) -> str: - """Lowercase + collapse whitespace. +def _canonical_gpr(reaction: cobra.Reaction) -> str: + """Canonical, order-insensitive form of a reaction's GPR, for logic-level comparison. + + DNF-expand the parsed GPR, then sort the genes within each isozyme clause and sort the clauses, so + operand order never registers as a difference: ``a and b`` == ``b and a`` and ``a or b`` == + ``b or a``. Mirrors MATLAB RAVEN's ``diffModels`` (`RAVEN #686 + `_), which compares grRules as logic rather than + text. Gene ids are lowercased and duplicate genes/clauses collapse, preserving the formatting-drift + tolerance the previous string heuristic gave. - A more robust comparator would parse to a GPR AST and compare - structures; this is the cheap heuristic that catches the formatting - drift we see between different SBML writers. + Falls back to that whitespace/lowercase heuristic for a rule cobra could not parse (``gpr.body`` is + ``None`` while the rule string is non-empty), so a malformed rule is still compared rather than + silently equated to the empty rule. """ - if not rule: - return "" - return " ".join(rule.lower().split()) + gpr = reaction.gpr + if gpr is not None and gpr.body is not None: + try: + clauses = gpr_to_dnf(gpr) + except ValueError: + clauses = None + if clauses is not None: + canon = sorted({tuple(sorted({g.lower() for g in clause})) for clause in clauses}) + return " | ".join(" & ".join(clause) for clause in canon) + return " ".join((reaction.gene_reaction_rule or "").lower().split()) def _normalise_annotation_value(v): diff --git a/tests/test_comparison_diff.py b/tests/test_comparison_diff.py index 91ea5f7..e4e2113 100644 --- a/tests/test_comparison_diff.py +++ b/tests/test_comparison_diff.py @@ -5,7 +5,7 @@ import pytest from raven_toolbox.comparison import DiffReport, diff_models -from raven_toolbox.comparison.diff import _normalise_gpr +from raven_toolbox.comparison.diff import _canonical_gpr def _mini_model(model_id: str = "m") -> cobra.Model: @@ -98,16 +98,55 @@ def test_extra_annotations_picked_up(): assert not diff_models(a, b, extra_annotations={"custom-key"}).equal +def _canon(rule: str) -> str: + r = cobra.Reaction("r") + r.gene_reaction_rule = rule + return _canonical_gpr(r) + + +@pytest.mark.parametrize( + "ga, gb", + [ + ("A and B", "a AND b"), # case + AND spelling + ("A and B", "A and B"), # whitespace + ("(A or B) and C", "(a OR b) AND c"), # case, nested + ("A and B", "B and A"), # AND operand order (the back-port) + ("A or B", "B or A"), # OR isozyme order (the back-port) + ("(A or B) and C", "C and (B or A)"), # nested, reordered at both levels + ("A and A", "A"), # duplicate gene collapses + ], +) +def test_canonical_gpr_is_order_insensitive(ga, gb): + assert _canon(ga) == _canon(gb) + + @pytest.mark.parametrize( "ga, gb", [ - ("A and B", "a AND b"), - ("A and B", "A and B"), - ("(A or B) and C", "(a OR b) AND c"), + ("A and B", "A or B"), # AND vs OR is a real logic difference, not formatting + ("A and B", "A and C"), # a different gene really differs ], ) -def test_gpr_normalisation(ga, gb): - assert _normalise_gpr(ga) == _normalise_gpr(gb) +def test_canonical_gpr_keeps_real_differences(ga, gb): + assert _canon(ga) != _canon(gb) + + +def test_diff_models_ignores_gpr_operand_order(): + # Two models identical but for GPR operand order must compare equal — the RAVEN diffModels + # behaviour this back-port adds. The string heuristic used to flag this as a GPR difference. + a = _mini_model("a") # r1 GPR is "g1 AND g2" + b = _mini_model("b") + b.reactions.r1.gene_reaction_rule = "g2 and g1" + assert diff_models(a, b).equal + + +def test_diff_models_flags_real_gpr_difference(): + a = _mini_model("a") # r1 GPR is "g1 AND g2" + b = _mini_model("b") + b.reactions.r1.gene_reaction_rule = "g1 or g2" # AND -> OR is genuine + report = diff_models(a, b) + assert not report.equal + assert any("GPR" in d for d in report.differences) def test_max_per_category_truncates():