From 31c94f4a136ca7cce8825623911544dae2df9580 Mon Sep 17 00:00:00 2001 From: Eduard Kerkhoven Date: Wed, 10 Jun 2026 01:03:51 +0200 Subject: [PATCH] Robustness and polish fixes Tier 4 of the review: small, targeted hardening, no behaviour change on valid input. - gapfilling/fill.py: clamp the connectivity gap-fill big-M to the largest finite bound magnitude, so a template reaction with an infinite bound no longer puts an infinite coefficient into the MILP (which broke the solver). - reconstruction/kegg/download.py: a malformed or unreadable .netrc now raises a ValueError explaining how to fix it, instead of a raw NetrcParseError/OSError. - io/excel.py: always write the metabolite formula to the METS COMPOSITION column; it was dropped whenever an InChI was present. - visualization: the empty stub package raises a clear NotImplementedError (with a roadmap pointer) on attribute access, via a PEP 562 module __getattr__. A regression test per fix. --- src/raven_python/gapfilling/fill.py | 17 +++++++++++++++-- src/raven_python/io/excel.py | 2 +- .../reconstruction/kegg/download.py | 9 ++++++++- src/raven_python/visualization/__init__.py | 14 ++++++++++++++ tests/test_gapfilling.py | 10 ++++++++++ tests/test_io_excel.py | 2 +- tests/test_reconstruction_kegg_download.py | 8 ++++++++ tests/test_visualization.py | 9 +++++++++ 8 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 tests/test_visualization.py diff --git a/src/raven_python/gapfilling/fill.py b/src/raven_python/gapfilling/fill.py index ad0f1ab..5f96fe2 100644 --- a/src/raven_python/gapfilling/fill.py +++ b/src/raven_python/gapfilling/fill.py @@ -15,6 +15,7 @@ """ from __future__ import annotations +import math from collections.abc import Iterable from dataclasses import dataclass @@ -76,13 +77,25 @@ def _solve_min_templates( prob = working.problem indicators: dict[str, object] = {} extra = [] + # Big-M for the indicator constraints. A template reaction with an infinite + # bound would otherwise put an infinite coefficient into the MILP; clamp it to + # the largest finite bound magnitude in the model (>= any flux it can carry). + finite_bounds = [ + abs(b) + for r in working.reactions + for b in (r.lower_bound, r.upper_bound) + if math.isfinite(b) + ] + big_m = max(finite_bounds) if finite_bounds else 1000.0 for rid in template_ids: rxn = working.reactions.get_by_id(rid) y = prob.Variable(f"_gf_keep_{rid}", type="binary") indicators[rid] = y # Flux is confined to [lb*y, ub*y]: zero unless the reaction is kept (y=1). - extra.append(prob.Constraint(rxn.flux_expression - rxn.upper_bound * y, ub=0, name=f"_gf_ub_{rid}")) - extra.append(prob.Constraint(rxn.flux_expression - rxn.lower_bound * y, lb=0, name=f"_gf_lb_{rid}")) + ub = rxn.upper_bound if math.isfinite(rxn.upper_bound) else big_m + lb = rxn.lower_bound if math.isfinite(rxn.lower_bound) else -big_m + extra.append(prob.Constraint(rxn.flux_expression - ub * y, ub=0, name=f"_gf_ub_{rid}")) + extra.append(prob.Constraint(rxn.flux_expression - lb * y, lb=0, name=f"_gf_lb_{rid}")) working.add_cons_vars(list(indicators.values()) + extra) if allow_net_production: # relax steady state to Sv >= 0 (mets may accumulate) diff --git a/src/raven_python/io/excel.py b/src/raven_python/io/excel.py index 398b6da..6e65c86 100644 --- a/src/raven_python/io/excel.py +++ b/src/raven_python/io/excel.py @@ -104,7 +104,7 @@ def export_to_excel( ws.append([ None, f"{m.name}[{m.compartment}]", m.name, None, _miriam_string(m.annotation, exclude=("smiles",)), - None if inchi else m.formula, inchi, m.compartment, m.id, m.charge, + m.formula, inchi, m.compartment, m.id, m.charge, ]) # --- COMPS --- diff --git a/src/raven_python/reconstruction/kegg/download.py b/src/raven_python/reconstruction/kegg/download.py index d990b42..f8ad930 100644 --- a/src/raven_python/reconstruction/kegg/download.py +++ b/src/raven_python/reconstruction/kegg/download.py @@ -69,7 +69,14 @@ def _resolve_auth( f"No credentials given and {path} does not exist. Create it (chmod 600) " f"with a line:\n machine {host} login YOUR_USER password YOUR_PASS" ) - creds = netrc.netrc(str(path)).authenticators(host) + try: + creds = netrc.netrc(str(path)).authenticators(host) + except (netrc.NetrcParseError, OSError) as exc: + raise ValueError( + f"Could not read credentials from {path}: {exc}. Ensure it is a valid " + f".netrc (chmod 600) with a line:\n" + f" machine {host} login YOUR_USER password YOUR_PASS" + ) from exc if not creds: raise ValueError( f"No credentials for '{host}' in {path}. Add a line:\n" diff --git a/src/raven_python/visualization/__init__.py b/src/raven_python/visualization/__init__.py index 4fb3839..613f1e2 100644 --- a/src/raven_python/visualization/__init__.py +++ b/src/raven_python/visualization/__init__.py @@ -1 +1,15 @@ """Pathway-map and omics-overlay visualisation (stub — not yet implemented).""" +from __future__ import annotations + + +def __getattr__(name: str): + # PEP 562: accessing any attribute of this stub package fails with a clear, + # actionable message rather than a bare AttributeError. Dunder lookups + # (``__path__`` etc.) fall through to normal handling. + if name.startswith("__"): + raise AttributeError(name) + raise NotImplementedError( + f"raven_python.visualization.{name!r} is not implemented yet. Pathway-map " + "and omics-overlay visualisation is on the roadmap; until then export the " + "model via raven_python.io and use an external viewer such as Escher." + ) diff --git a/tests/test_gapfilling.py b/tests/test_gapfilling.py index 1922fea..0380c68 100644 --- a/tests/test_gapfilling.py +++ b/tests/test_gapfilling.py @@ -54,6 +54,16 @@ def test_fill_gaps_connects_blocked_reaction(draft_and_template): assert "r_unneeded" not in res.added_reactions # irrelevant template rxn not added +def test_fill_gaps_handles_infinite_template_bound(draft_and_template): + # A template reaction with an unbounded (inf) bound must not put an infinite + # coefficient into the big-M MILP; it is clamped to a finite surrogate. + draft, template = draft_and_template + template.reactions.get_by_id("r2").upper_bound = float("inf") + res = connect_blocked_reactions(draft, template) + assert "r1" in res.newly_connected + assert "r2" in res.added_reactions + + def test_fill_gaps_returns_working_model_that_unblocks(draft_and_template): draft, template = draft_and_template res = connect_blocked_reactions(draft, template) diff --git a/tests/test_io_excel.py b/tests/test_io_excel.py index 12434ef..08c9d5b 100644 --- a/tests/test_io_excel.py +++ b/tests/test_io_excel.py @@ -76,7 +76,7 @@ def test_mets_sheet(model, tmp_path): assert atp["ID"] == "ATP[c]" assert atp["NAME"] == "ATP" assert atp["InChI"] == "InChI=1S/X" - assert atp["COMPOSITION"] is None # suppressed when InChI present + assert atp["COMPOSITION"] == "C10H16N5O13P3" # formula kept even when InChI present assert atp["CHARGE"] == -4 assert atp["MIRIAM"] == "kegg.compound/C00002" # smiles excluded diff --git a/tests/test_reconstruction_kegg_download.py b/tests/test_reconstruction_kegg_download.py index 38d2f44..54a7af1 100644 --- a/tests/test_reconstruction_kegg_download.py +++ b/tests/test_reconstruction_kegg_download.py @@ -57,6 +57,14 @@ def test_resolve_auth_host_absent(tmp_path): _resolve_auth("ftp.kegg.net", netrc_path=netrc_file) +def test_resolve_auth_malformed_netrc(tmp_path): + netrc_file = tmp_path / ".netrc" + netrc_file.write_text("this is not a valid netrc line\n") + netrc_file.chmod(0o600) + with pytest.raises(ValueError, match="Could not read credentials"): + _resolve_auth("ftp.kegg.net", netrc_path=netrc_file) + + # --------------------------------------------------------------------------- # # Extract / arrange # --------------------------------------------------------------------------- # diff --git a/tests/test_visualization.py b/tests/test_visualization.py new file mode 100644 index 0000000..5cc2cef --- /dev/null +++ b/tests/test_visualization.py @@ -0,0 +1,9 @@ +"""The visualization subpackage is a documented stub (not yet implemented).""" +import pytest + + +def test_visualization_stub_raises_not_implemented(): + import raven_python.visualization as viz + + with pytest.raises(NotImplementedError, match="not implemented yet"): + viz.draw_pathway() # attribute access triggers __getattr__ and raises