fix: inverted allowExcretion constraint sense in ftINITInternalAlg - #675
Merged
Conversation
allowExcretion set csense 'L' on the metabolite rows, i.e. S*v <= 0. That
allows a metabolite to be consumed without ever being produced -- free
uptake, the exact opposite of excretion, and enough for the MILP to switch a
reaction on by conjuring its substrates out of nothing. Since ftINIT locks
step-1 picks in as essentialRxns, such a reaction is then kept for the rest
of the run.
The intended constraint is S*v >= 0 ('G'): a metabolite may be produced in
excess of what is consumed, and the surplus is implicitly excreted. This is
what the function's own docstring describes, what the parameter name says,
and what runINIT does for the same flag -- it sets the metabolite row's
upper bound to inf (metUB=inf(nMets,1)) in the slack formulation, which is
S*v >= 0.
Verified against optimizeProb directly: with a single row S*v and one
producing reaction, csense 'L' gives max v = 0 (production forbidden) and
'G' gives max v = 10 (production allowed).
Also give the else branch one csense character per row. A scalar '=' only
works because gurobi broadcasts it; optimizeProb maps csense elementwise for
glpk ('E'->'S') and cobra, so the scalar does not survive dispatch.
Honest scope note: the synthetic model in tINIT does not discriminate
between the two senses -- it has exchange reactions, which mask the effect,
and '1+1', '2+1' and '2+0' all return the same reactions before and after
this change. The fix rests on the constraint semantics above, not on an
observed output change, and no test in the suite regresses.
Tests: only '1+1' and 'full' were ever exercised, so the paper's 2-step
series had no coverage. Adds ftINITSeriesVariantsRun pinning '2+1' and
'2+0'; '2+0' keeps the GPR-less transport R2, as its docstring says it
should.
Function test results241 tests 218 ✅ 1m 2s ⏱️ Results for commit 334cbf0. |
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:
ftINITInternalAlgappliedallowExcretionwith an inverted constraint sense, permitting free uptake instead of secretion. Affects'2+1'and'2+0', and changes their output.prob.csensewas broadcast in a Gurobi-only form that broke glpk/cobra dispatch.'2+1'coverage intINIT.m.Instructions on merging this PR:
develop3as target branch, and will be resolved with a squash-merge.