From 6095f6b6b37ce9bac14de5436199c52767f7d60e Mon Sep 17 00:00:00 2001 From: Eduard Kerkhoven Date: Wed, 21 Oct 2026 06:24:54 +0000 Subject: [PATCH 1/3] fix: keep everything of every model in merge_models; name the subsystem of transport reactions merge_models added only the metabolites and genes that a reaction uses, dropped the objective, gave a compartment name to the last model that named it, and ordered new metabolites and genes by the reactions that reached them. RAVEN's mergeModels keeps all metabolites, genes and compartments of every model, concatenates the objective, keeps the first model's compartment name, and appends new items in the order of their source model. Confirmed on MATLAB (RAVEN 79a7d8c): an unused metabolite and gene survive and both objectives are kept. add_transport_reactions left the subsystem empty. RAVEN's addTransport sets "Transport between and " (reversible) or "Transport from to " (irreversible), using the compartment ids; confirmed on MATLAB for c and m. --- src/raven_toolbox/manipulation/merge.py | 29 ++++- src/raven_toolbox/manipulation/transport.py | 10 +- tests/test_manipulation_merge.py | 119 ++++++++++++++++++++ tests/test_manipulation_transport.py | 17 +++ 4 files changed, 171 insertions(+), 4 deletions(-) diff --git a/src/raven_toolbox/manipulation/merge.py b/src/raven_toolbox/manipulation/merge.py index 175842bc..83665624 100644 --- a/src/raven_toolbox/manipulation/merge.py +++ b/src/raven_toolbox/manipulation/merge.py @@ -5,7 +5,10 @@ compound under different ids in two models becomes one), while adding **all** reactions without de-duplication (a reaction whose ID already exists is renamed ``id_``). Genes are -unified by ID. Provenance (which source model each object came from) is recorded +unified by ID. Like RAVEN, every metabolite and gene of every model is kept, also +when no reaction uses it; the objective of every model is carried over; when two +models name the same compartment differently the first model's name is kept; new +metabolites and genes are added in the order of their source model. Provenance (which source model each object came from) is recorded in ``notes['origin']``. The bulk of RAVEN's function is struct field-padding and manual S-matrix @@ -18,7 +21,7 @@ from collections.abc import Iterable import cobra -from cobra import Metabolite, Model, Reaction +from cobra import Gene, Metabolite, Model, Reaction def _unique_id(existing, base: str, suffix: str) -> str: @@ -69,7 +72,9 @@ def merge_models( return models[0].copy() merged = Model("MERGED") + merged.objective_direction = models[0].objective_direction comp_names: dict[str, str] = {} + objective: dict[Reaction, float] = {} met_lookup: dict = {} # name/comp or id key -> merged Metabolite def met_key(met: Metabolite): @@ -118,9 +123,23 @@ def ensure_metabolite(src: Metabolite, origin: str) -> Metabolite: for model in models: origin = model.id or "model" - comp_names.update(model.compartments) + # model.compartments lists only compartments that hold metabolites; RAVEN keeps all. + for comp, name in {**model.compartments, **model._compartments}.items(): + comp_names.setdefault(comp, name) # the first model's name for a compartment wins genes_before = {g.id for g in merged.genes} + # Every metabolite and gene of the model, in the model's order, before the reactions + # that use them: unused ones are kept, and the order does not depend on the reactions. + for met in model.metabolites: + ensure_metabolite(met, origin) + for gene in model.genes: + if gene.id not in merged.genes: + new_gene = Gene(gene.id, name=gene.name) + new_gene.annotation = copy.deepcopy(gene.annotation) + new_gene.notes = copy.deepcopy(gene.notes) + merged.genes.append(new_gene) + new_gene._model = merged + new_rxns: list[Reaction] = [] for rxn in model.reactions: new_id = _unique_id(merged.reactions, rxn.id, origin) @@ -140,6 +159,8 @@ def ensure_metabolite(src: Metabolite, origin: str) -> Metabolite: new_rxn.notes = copy.deepcopy(rxn.notes) if track_origin: new_rxn.notes.setdefault("origin", origin) + if rxn.objective_coefficient: + objective[new_rxn] = rxn.objective_coefficient if track_origin: for gene in merged.genes: @@ -147,4 +168,6 @@ def ensure_metabolite(src: Metabolite, origin: str) -> Metabolite: gene.notes.setdefault("origin", origin) merged._compartments.update(comp_names) + if objective: + merged.objective = objective return merged diff --git a/src/raven_toolbox/manipulation/transport.py b/src/raven_toolbox/manipulation/transport.py index e3c717e6..a544cbcf 100644 --- a/src/raven_toolbox/manipulation/transport.py +++ b/src/raven_toolbox/manipulation/transport.py @@ -4,7 +4,10 @@ species by *name* across compartments (the source in ``from_compartment`` and its same-named twin in each target compartment), optionally creating the target metabolite, and -builds a ``-1 from / +1 to`` reaction with a sequential ``tr_0001`` ID. +builds a ``-1 from / +1 to`` reaction with a sequential ``tr_0001`` ID. As in RAVEN's +``addTransport``, the subsystem of each reaction names the two compartments: +``Transport between and `` for a reversible reaction and +``Transport from to `` for an irreversible one (the compartment ids are used). """ from __future__ import annotations @@ -159,6 +162,11 @@ def add_transport_reactions( rxn = Reaction(next_id()) rxn.name = f"{name} transport, {from_name}-{to_name}" rxn.bounds = bounds + rxn.subsystem = ( + f"Transport between {from_compartment} and {to_comp}" + if reversible + else f"Transport from {from_compartment} to {to_comp}" + ) added.append(rxn) pending_coeffs.append({src: -1, dst: 1}) diff --git a/tests/test_manipulation_merge.py b/tests/test_manipulation_merge.py index b789c960..251e9c5e 100644 --- a/tests/test_manipulation_merge.py +++ b/tests/test_manipulation_merge.py @@ -134,3 +134,122 @@ def test_charge_conflict_warns(): [{"id": "EX_B", "equation": "g2 -->"}]) with pytest.warns(UserWarning, match="different charges"): merge_models([a, b]) + + +# --- RAVEN's mergeModels keeps everything of every model ------------------------------ + +def _with_extras(): + """Model with a metabolite, a gene and a compartment that no reaction uses.""" + m = _model( + "X", + [cobra.Metabolite("a_c", name="A", compartment="c"), + cobra.Metabolite("b_c", name="B", compartment="c")], + [{"id": "R1", "equation": "a_c --> b_c", "gene_reaction_rule": "G1"}], + ) + m.add_metabolites([cobra.Metabolite("orph_c", name="Orphan", compartment="c")]) + orphan = cobra.Gene("G_ORPH", name="unused gene") + m.genes.append(orphan) + orphan._model = m + return m + + +def test_unused_metabolites_and_genes_are_kept(model_b): + merged = merge_models([_with_extras(), model_b]) + assert "orph_c" in merged.metabolites + assert "G_ORPH" in merged.genes + assert merged.genes.get_by_id("G_ORPH").name == "unused gene" + assert merged.genes.get_by_id("G_ORPH").notes["origin"] == "X" + + +def test_unused_items_of_a_later_model_are_kept_too(model_a): + merged = merge_models([model_a, _with_extras()]) + assert "orph_c" in merged.metabolites + assert "G_ORPH" in merged.genes + + +def test_objective_of_every_model_is_carried_over(model_a, model_b): + model_a.objective = "HEX" + model_b.objective = {model_b.reactions.get_by_id("LDH"): 2.0} + merged = merge_models([model_a, model_b]) + coefficients = {r.id: r.objective_coefficient for r in merged.reactions if r.objective_coefficient} + assert coefficients == {"HEX": 1.0, "LDH": 2.0} + + +def test_objective_follows_a_renamed_reaction(model_a): + other = _model( + "B", + [cobra.Metabolite("glc_c", name="Glucose", compartment="c"), + cobra.Metabolite("x_c", name="X", compartment="c")], + [{"id": "HEX", "equation": "glc_c --> x_c"}], + ) + other.objective = "HEX" + merged = merge_models([model_a, other]) + assert merged.reactions.get_by_id("HEX_B").objective_coefficient == 1.0 + assert merged.reactions.get_by_id("HEX").objective_coefficient == 0 + + +def test_no_objective_stays_empty(model_a, model_b): + merged = merge_models([model_a, model_b]) + assert not any(r.objective_coefficient for r in merged.reactions) + + +def test_objective_direction_comes_from_the_first_model(model_a, model_b): + model_a.objective = "HEX" + model_a.objective_direction = "min" + merged = merge_models([model_a, model_b]) + assert merged.objective_direction == "min" + + +def test_first_model_names_a_compartment(): + first = _model("A", [cobra.Metabolite("a_c", name="A", compartment="c")], []) + first.compartments = {"c": "cytoplasm"} + second = _model("B", [cobra.Metabolite("b_c", name="B", compartment="c")], []) + second.compartments = {"c": "cytosol", "e": "extracellular"} + merged = merge_models([first, second]) + assert merged.compartments["c"] == "cytoplasm" + assert merged._compartments["e"] == "extracellular" # a compartment without metabolites is kept too + + +def test_order_follows_the_source_models(model_a, model_b): + # model_b lists lac_c, then glucose_c: the merged order is model_a's metabolites, + # then the new ones of model_b in model_b's order (RAVEN appends in model order). + merged = merge_models([model_a, model_b]) + assert [m.name for m in merged.metabolites] == ["Glucose", "G6P", "Lactate"] + assert [g.id for g in merged.genes] == ["GA", "GB"] + + +def test_order_does_not_depend_on_the_reactions(): + # The reaction uses the second metabolite first; the model order is still kept. + m = cobra.Model("O") + m.add_metabolites([cobra.Metabolite("p_c", name="P", compartment="c"), + cobra.Metabolite("q_c", name="Q", compartment="c")]) + add_reactions_from_equations(m, [{"id": "R", "equation": "q_c --> p_c", "gene_reaction_rule": "G2 or G1"}]) + merged = merge_models([m, m.copy()]) + assert [x.id for x in merged.metabolites][:2] == ["p_c", "q_c"] + + +def test_merge_is_independent_of_the_hash_seed(): + import subprocess + import sys + code = ( + "import cobra\n" + "from raven_toolbox.manipulation import merge_models\n" + "ms = []\n" + "for i in range(2):\n" + " m = cobra.Model(f'M{i}')\n" + " m.add_metabolites([cobra.Metabolite(f'm{j}_c', name=f'N{j}', compartment='c') for j in range(6)])\n" + " for k in (5, 3, 4, 1, 2, 0):\n" + " g = cobra.Gene(f'G{k}'); m.genes.append(g); g._model = m\n" + " r = cobra.Reaction(f'R{i}'); m.add_reactions([r])\n" + " r.add_metabolites({m.metabolites[0]: -1, m.metabolites[1]: 1})\n" + " r.gene_reaction_rule = ' or '.join(f'G{k}' for k in (5, 3, 4, 1, 2, 0))\n" + " ms.append(m)\n" + "g = merge_models(ms)\n" + "print([x.id for x in g.metabolites], [x.id for x in g.genes])\n" + ) + outputs = set() + for seed in ("0", "1", "2"): + out = subprocess.run([sys.executable, "-I", "-c", code], capture_output=True, text=True, + env={"PYTHONHASHSEED": seed, "PATH": ""}, check=True).stdout + outputs.add(out) + assert len(outputs) == 1 diff --git a/tests/test_manipulation_transport.py b/tests/test_manipulation_transport.py index 6a5b83b9..d4cdaaf1 100644 --- a/tests/test_manipulation_transport.py +++ b/tests/test_manipulation_transport.py @@ -96,3 +96,20 @@ def test_duplicate_name_in_source_compartment_warns(model): added = add_transport_reactions(model, "c", "m", ["H2O"], only_to_existing=False) # Transport still works (uses the first match) — the warning is the signal. assert len(added) == 1 + + +# --- the subsystem names the two compartments, as in RAVEN's addTransport ----------------- + +def test_reversible_transport_subsystem(model): + added = add_transport_reactions(model, "c", "m", ["ATP"]) + assert added[0].subsystem == "Transport between c and m" + + +def test_irreversible_transport_subsystem(model): + added = add_transport_reactions(model, "c", "m", ["ATP"], reversible=False) + assert added[0].subsystem == "Transport from c to m" + + +def test_subsystem_names_each_target_compartment(model): + added = add_transport_reactions(model, "c", ["m", "e"], ["ATP"], only_to_existing=False) + assert [r.subsystem for r in added] == ["Transport between c and m", "Transport between c and e"] From 6c9775647d662f56d3279efcda1d207ad0041ec9 Mon Sep 17 00:00:00 2001 From: Eduard Kerkhoven Date: Wed, 21 Oct 2026 10:48:36 +0000 Subject: [PATCH 2/3] fix: set the hash seed explicitly in the merge_models order test The test passed -I, which makes Python ignore PYTHONHASHSEED, and replaced the whole environment, which stops Python from starting on Windows (no SYSTEMROOT). It now keeps the environment and sets the seed. --- tests/test_manipulation_merge.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_manipulation_merge.py b/tests/test_manipulation_merge.py index 251e9c5e..e4ce91c4 100644 --- a/tests/test_manipulation_merge.py +++ b/tests/test_manipulation_merge.py @@ -229,6 +229,7 @@ def test_order_does_not_depend_on_the_reactions(): def test_merge_is_independent_of_the_hash_seed(): + import os import subprocess import sys code = ( @@ -249,7 +250,9 @@ def test_merge_is_independent_of_the_hash_seed(): ) outputs = set() for seed in ("0", "1", "2"): - out = subprocess.run([sys.executable, "-I", "-c", code], capture_output=True, text=True, - env={"PYTHONHASHSEED": seed, "PATH": ""}, check=True).stdout + # The seed is set explicitly (not "-I", which ignores PYTHON* variables), and the rest + # of the environment is kept: Python does not start on Windows without SYSTEMROOT. + out = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True, + env={**os.environ, "PYTHONHASHSEED": seed}, check=True).stdout outputs.add(out) assert len(outputs) == 1 From c170c9e381a5437092bf1d4d48f1bee891da70da Mon Sep 17 00:00:00 2001 From: Eduard Kerkhoven Date: Wed, 21 Oct 2026 10:48:45 +0000 Subject: [PATCH 3/3] ci: upgrade pip with python -m pip pip 26.2.2 refuses to replace itself on Windows when run as plain pip and exits 1, so every Windows job failed before running any test. --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d8082da2..d9492027 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,7 @@ jobs: python-version: "3.12" cache: pip cache-dependency-path: pyproject.toml - - run: pip install --upgrade pip + - run: python -m pip install --upgrade pip # Pinned so the lint result is reproducible (unpinned ruff silently # adds rules over time). Keep in sync with the ``dev`` extra in pyproject. - run: pip install ruff==0.15.15 @@ -38,7 +38,7 @@ jobs: python-version: "3.12" cache: pip cache-dependency-path: pyproject.toml - - run: pip install --upgrade pip + - run: python -m pip install --upgrade pip # Installs runtime deps + the dev extra (mypy). Third-party stubs are # absent for cobra/optlang/scipy; ignore_missing_imports handles that. - run: pip install -e ".[dev]" @@ -65,7 +65,7 @@ jobs: python-version: ${{ matrix.python }} cache: pip cache-dependency-path: pyproject.toml - - run: pip install --upgrade pip + - run: python -m pip install --upgrade pip # ``-e .[dev,excel]`` so every optional extra is exercised. # Gurobi is not installable on free runners; the relevant tests # skip themselves when ``optlang.gurobi_interface`` cannot import. @@ -98,7 +98,7 @@ jobs: python-version: "3.12" cache: pip cache-dependency-path: pyproject.toml - - run: pip install --upgrade pip + - run: python -m pip install --upgrade pip # gurobipy ships a size-limited licence (2000 variables / 2000 # constraints). That is useless at genome scale but ample for the small # fixtures here, and it gives the cross-solver check a second solver to