fix(writePajekNet): directed Pajek output, tolerance filter, robust filtering & checks#2651
Conversation
…iltering & checks - Emit *Arcs (directed) instead of *Edges, matching the actual semantics of the encoded graph (forward/reverse based on flux sign). - Replace exact-zero flux check with a configurable fluxTol (defaults to the LP solver feasTol via getCobraSolverParams) so solver residuals do not produce spurious arcs. - Validate the FBA solution status before consuming FBA.full; raise writePajekNet:lpFailed on infeasible / unbounded / failed solves. - Make biomass / objective filtering case-insensitive and also exclude any reaction with a non-zero linear objective (model.c). - Use fprintf format specifiers, guarantee fclose via onCleanup, and expose an optional fileName argument. - Rewrite testwritePajekNet to assert directed format, tolerance behaviour, biomass exclusion, and the LP-failure error path; remove the obsolete byte-equal fixture (COBRAmodeltest.net).
Summary:
Failed Tests:No failed tests ✨ Github Test Reporter by CTRF 💚 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5935dba7c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for w = excludedFluxes(:)' | ||
| assert(~any(abs(arcWeights - w) < 1e-12), ... | ||
| 'Excluded reaction with flux %g leaked into the graph', w); |
There was a problem hiding this comment.
This check assumes each excluded reaction has a unique flux magnitude, but in ecoli_core_model many non-excluded reactions share the same absolute flux as the biomass/objective reaction (for example, repeated 0.87392 values), so the test can fail even when writePajekNet correctly excludes those reactions. Because arc weights are not reaction identifiers, this assertion is fundamentally unstable across models/solvers and will produce false negatives; the test should compare expected reaction-derived arcs (or reaction membership) instead of raw weight equality.
Useful? React with 👍 / 👎.
Codex P1 on opencobra#2651: the previous exclusion check asserted that no arc weight matched any excluded reaction's flux magnitude. In ecoli_core_model many non-excluded reactions share the biomass reaction's |v| (e.g. 0.87392), so the assertion is unstable across models and solvers and can produce false negatives. Replace it with a multiset comparison: replicate writePajekNet's filtering (biomass / objective name match, model.c != 0, |v| < fluxTol, demand/sink) to build the expected (src, dst, weight) arc list, parse the file's actual arcs, sort both, and assert equality. This validates exclusion by reaction identity and additionally catches missing arcs, swapped src/dst, and wrong weights. Expected weights are round-tripped through writePajekNet's '%g' format so the comparison is exact. Also closes a small gap in the previous test, which only matched 'biomass' in reaction names; now matches 'objective' as well, aligning with writePajekNet's own logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary:
Failed Tests:No failed tests ✨ Github Test Reporter by CTRF 💚 |
2 participants
Summary
Audit of
writePajekNetsurfaced five issues. This PR fixes all of them and rewrites the regression test to assert semantics rather than byte-equality with a frozen fixture.Fixes
*Arcsinstead of*Edges) — the function already encodes direction (forward/reverse based on flux sign), but the previous header marked the file as undirected, so downstream tools could discard the directionality.FBA.full(i) == 0withabs(v(i)) < fluxTol.fluxToldefaults to the LP solver'sfeasTol(viagetCobraSolverParams) and can be overridden via the new optional argument. Prevents tiny solver residuals (e.g. 1e-12) from being serialised as real arcs.FBA.stat == 1before consumingFBA.full. On failure, raisewritePajekNet:lpFailedinstead of silently emitting a graph from an invalid solution or crashing on indexing.Biomass_*no longer leaks through) and additionally exclude any reaction whose linear-objective coefficient (model.c) is non-zero..netbyte-for-byte against a frozen fixture, so any of the bugs above could pass. The new test:*Verticesblock matchesmodel.mets,*Arcsheader (directed) with a count matching the number of arc lines,fluxTol⇒ zero arcs),writePajekNet:lpFailed).The obsolete
COBRAmodeltest.netfixture is removed.Backward compatibility
writePajekNet(model)still works; new arguments are optional..net; consumers that ignored the previous*Edgesvs*Arcsdistinction are unaffected, while consumers that respect it now get the correct semantics.fluxTolmatches the configured solver tolerance, so any reaction the solver itself considers active is still emitted.Files
src/base/io/utilities/writePajekNet.m— fixes 1–4, plus optionalfluxTolandfileNamearguments andonCleanup-guardedfclose.test/verifiedTests/base/testwritePajekNet/testwritePajekNet.m— semantic test (fix 5).test/verifiedTests/base/testwritePajekNet/COBRAmodeltest.net— removed (obsolete fixture).