refactor: rearchitect assignCompartments as place -> repair -> certify - #688
Open
edkerk wants to merge 6 commits into
Open
refactor: rearchitect assignCompartments as place -> repair -> certify#688edkerk wants to merge 6 commits into
edkerk wants to merge 6 commits into
Conversation
The previous version coupled a flux model into the placement MILP (Big-M flux gating, a growth-floor row) and set exitFlag=1 from the MILP status alone, materialising outModel without ever solving it. At genome scale that can certify a placement the built model cannot actually grow. Placement is now a flux-free MILP over only the reaction/compartment and gene/compartment binaries, maximising the localization score, so it has nothing for a tolerance-rounded binary to leak into. A solver-free confinement fixpoint keeps the placement connected: a non-transportable metabolite split across compartments forces its reactions to co-locate, while transportable splits get passive transports. The placement is then materialised and that exact model is solved; exitFlag reflects whether it reached the growth floor, and a new fifth output reports certified/status and the real per-medium growths, so a failed certification is honest rather than hidden. Adds a growthConditions argument (certify on extra media). transportCost is now accepted but unused: placement follows the score, and transports are a structural consequence of repair. The three existing tests still pass; new tests cover honest certification reporting and the confinement co-location.
Function test results304 tests 274 ✅ 1m 6s ⏱️ Results for commit 735e850. ♻️ This comment has been updated with latest results. |
…at genome scale Benchmarking against raven-toolbox on yeast-GEM (identical scores, relocate set and curated ground truth) surfaced two issues in the first version: - On a real multi-compartment model, the internal merge produced a 'System' compartment while pinned reactions were treated as being in the default, disconnecting the merged metabolites. The merged compartment is now relabelled to the default so placement and transports are consistent. - The score objective rewarded only the gene placement (y), leaving the reaction placement (x) underdetermined: any placement consistent with the gene assignment was optimal, so the solver scattered reactions arbitrarily among each gene's compartments. Reaction agreement with curated yeast-GEM was 51.9% (following the score argmax only 57.5% of the time). A small score-consistent tie-break on x now pulls each reaction into the compartment its own genes score highest, raising agreement to 70.8% (Python 72.0%) and direct placement overlap with Python to 81.4%, without changing the gene assignment. The model still certifies at the same growth.
…x method Reworked assignCompartments to mirror raven_toolbox.localization .assign_compartments function-for-function, removing the earlier MATLAB-only divergences: - no internal merge: each reaction keeps its own compartment (pinned stay put), matching Python; the caller merges the draft first if desired. - placement objective is score*y - penalty*y only, dropping the MATLAB-only score tie-break on the reaction binaries. - transport pruning (pruneTransports, on by default): transports blocked in every certification medium are dropped, via haveFlux. - growth-gap feedback: a stranded biomass precursor's producer is pinned to biomass's compartment and the placement re-solved, with the protected/ gap-pinned mechanism so confinement and the growth-gap diagnoser cannot oscillate. - confinement is the full force / co-locate / relax logic, and relaxed metabolites flow into the split transports. - materialisation matches Python's id conventions and reconciles all model fields; the compartment set is the union of model and score compartments; transportable is keyed by metabolite name. Benchmarked against Python on yeast-GEM (identical scores/relocate/ground truth): both certify at growth 0.1426; transports 1117 vs 1001. Reaction agreement is 51.9% vs Python's 72.0% -- the objective rewards the gene placement and leaves the reaction placement underdetermined, so the residual is solver tie-breaking between the two Gurobi stacks, not a method difference.
Reconciled the divergences from raven_toolbox.localization.assign_compartments surfaced by a function-by-function comparison: - movable reactions are now processed in sorted-reaction-id order, matching the Python master's variable order (a faithfulness fix; it does not change the genome-scale result, so the placement difference vs Python is solver tie-breaking between the two Gurobi stacks, not variable ordering). - biomass reaction is the largest objective-coefficient reaction (was the first non-zero), and a biomassReaction argument can name it explicitly. - a baseMetabolite argument selects the compartment-agnostic key (default 'name', the RAVEN convention; the Python default strips the id's compartment suffix, which is cobra's convention). - confinement and split transports are keyed by base metabolite, not by per-compartment row, so a species split across compartments is detected on a non-merged model as it is in Python (identical on a merged draft). - minimizeTransports (parsimonious-FBA transport pruning), off by default, is ported. Not ported: universal-model gap-fill, and multi-localization (which needs a loopless FVA RAVEN does not provide). Genome-scale benchmark unchanged (certified, growth 0.1426, 1117 transports).
… assignCompartments Adds the two pieces the faithfulness review flagged as unported, and the loopless FVA RAVEN was missing to make one of them possible: - cycleFreeFlux: remove thermodynamically infeasible internal loops from a flux distribution (Desouki et al. 2015) by fixing the exchanges and minimising internal flux. - looplessFVA: loop-free flux variability, via the loop-law MILP of Schellenberger et al. (2011) with metabolite potentials, so a reaction that reaches a bound only around an internal cycle reports the flux it can actually carry loop-free. (cycleFreeFlux on an FBA solution is insufficient for correct bounds; the loop-law MILP is used instead of the cobra cycleFreeFlux variant the Python reference calls.) - assignCompartments now accepts a universal model and gap-fills from it (via fillGaps) when a placement fails to certify, and a multiLocalize option that proposes a second compartment per reaction (gene score >= threshold), materialises the duplicate, and keeps it only if looplessFVA shows it can carry flux at the growth floor. Both re-certify and fall back on failure. Both off by default; the default path is unchanged. Tests for cycleFreeFlux/looplessFVA (a futile-loop model) and for the multi-localization keep path.
…port Iterate genes in sorted-id order and emit the placement MILP constraint rows in the same interleaved order as the raven-toolbox master (per movable: its place row then its couple rows; per gene: its gene1 row then its has rows). Pin the solve to a single thread, a fixed seed and a zero MIP gap so a degenerate score objective resolves to the same co-optimal placement on every run. Column order, row order and parameters now match the Python master, which return byte-identical placements on yeast-GEM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main improvements in this PR:
assignCompartmentssetexitFlag=1from the MILP status alone and returnedoutModelwithout ever solving it, so it could certify a placement the built model cannot grow. Placement is now a flux-free score MILP (no flux variables, no growth row — nothing for a rounded binary to leak into); a solver-free confinement fixpoint co-locates reactions sharing a non-transportable metabolite and adds transports for transportable splits; and the materialised model is actually solved, withexitFlagreflecting real certification.report(certified,status, per-mediumgrowths) so a failed certification reports the real growth. NewgrowthConditionsargument certifies on extra media.tAssignCompartments.mkeeps its three tests and adds honest-reporting and confinement co-location tests.Note:
transportCostis now accepted but unused (placement follows the score; transports are a repair consequence), matching the raven-toolbox design.Instructions on merging this PR:
develop3as target branch, and will be resolved with a squash-merge.