Bill knock-ins once, and stop them doubling as knockout candidates - #80
Open
VonAlphaBisZulu wants to merge 3 commits into
Open
Bill knock-ins once, and stop them doubling as knockout candidates#80VonAlphaBisZulu wants to merge 3 commits into
VonAlphaBisZulu wants to merge 3 commits into
Conversation
Two remaining consequences of a reaction sitting in both cost dictionaries at once. Neither can arise for a knockout-only problem, which is why the existing tests do not reach them. SDSolutions._compute_costs_and_bounds walked each cost dictionary in turn and added whatever it found, so a reaction present in both was billed twice. With ki_cost given for four reactions and ko_cost defaulting to all of them, a three-intervention design was reported at cost 6. It now selects the dictionary by the sign of the entry -- positive for an addition, negative for a removal -- which is how SDProblem prices interventions and how filter_sd_maxcost selects a cost. Design sets are untouched; only reported costs change, and only where the dictionaries overlapped. compute_strain_designs defaulted ko_cost to every reaction in the model whenever the caller gave no knockout costs, including reactions the caller had named as knock-in candidates. Knock-ins override knockouts in the MILP, so those entries described a state that could never be solved. Worse, with every reaction named addable, a SUPPRESS problem still answered with knockouts the caller had not asked for. The default now skips the named knock-ins. Reactions the caller did not name stay knockable, so mixed problems -- "R2 may be added, anything else may be removed" -- behave exactly as before. Four regression tests cover the pair, over every installed solver: elementary flux vector enumeration through a PROTECT module with all reactions addable, a negative cost forcing a reaction into every design, single billing of an intervention, and knock-in candidates not being offered as knockouts. Twelve of the sixteen fail without these changes. e_coli_core MCS (growth >= 0.001, max_cost 3) returns the identical 353 designs. Test suite: 372 passed, 2 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFtof9nZXFvCNoXz19po8C
Contributor
Author
|
Heavy design gates run locally (these are behind Both the 455 and 393 design gates hold on both solvers. |
…signs by cost rather than set inclusion Compression could invent designs and lose them. A parallel lump mixing a candidate with a non-targetable reaction let the lump's binary force flux that no intervention can remove, so a knockout was reported that cuts nothing; and the knock-in cost guard compared original reaction ids against a vector already re-keyed by lump names, so it never matched and a mixed lump became a knock-in instead of a knockout. Reaction targetability now takes part in the parallel key, and the guards read the vectors as they entered the compression step. With compress=False the compressed cost vectors aliased the uncompressed ones, which then had essential and size-1-MCS reactions popped out of them. filter_sd_maxcost priced every knockout at zero from there on, so max_cost was not enforced. Designs were selected by set inclusion, which agrees with cost only while every intervention is expensive. A free or rewarding intervention makes the larger design the cheaper one, so a design is now reported when no comparable design costs strictly less: unchanged for positive costs, both variants kept at zero cost, and the design taking a reward preferred over the one without it. Where a reward provably cannot invalidate a design -- a knock-in with only PROTECT modules, a knockout with only SUPPRESS ones -- it is required by a constraint row instead of being discovered, which keeps the enumeration from walking the designs it dominates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFtof9nZXFvCNoXz19po8C
Both ways of saying "this intervention is part of every design" are equivalent to gurobi, cplex and glpk, but with the bound form SCIP's populate re-reports the same design instead of reporting infeasible once the designs are exhausted, so enumeration never terminates. Measured head to head with only the form varying. link_z's step 7 skips inverted z-variables, so nothing downstream relied on the pinned bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFtof9nZXFvCNoXz19po8C
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
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.
Follow-up to #79. Two remaining consequences of a reaction sitting in both cost dictionaries at once. Neither can arise for a knockout-only problem, which is why the existing tests never reached them.
1. Interventions were billed once per dictionary
SDSolutions._compute_costs_and_boundswalked each cost dictionary in turn and added whatever it found, so a reaction present in both was charged twice:{R1,R2,R4}, allki_cost1It now selects the dictionary by the sign of the entry — positive for an addition, negative for a removal — matching how
SDProblemprices interventions and howfilter_sd_maxcostselects a cost.Design sets are untouched; only reported costs change, and only where the dictionaries overlapped.
2. Knock-in candidates were also offered as knockouts
compute_strain_designsdefaultedko_costto every reaction in the model whenever the caller gave no knockout costs — including reactions the caller had just named as knock-in candidates. Knock-ins override knockouts in the MILP (SDProblemmasksko_costwhereverki_costis set), so those entries described a state that could never be solved.The visible consequence: a SUPPRESS problem with every reaction named addable still answered with knockouts nobody asked for.
The default now skips the named knock-ins. Reactions the caller did not name stay knockable, so mixed problems behave exactly as before —
tests/test_05_straindesign.py::test_mcspasseski_cost={'R2': 1}and expects knockout designs on the other reactions, and that still holds.For the record, the stronger variant — emptying
ko_costentirely when knock-ins are present — was tried and broke 25 parametrisations of that test, so it is deliberately not what this does.Regression tests
Four new tests in
tests/test_05_straindesign.py, parametrised over every installed solver:test_efv_enumeration_via_knockins— a PROTECT module with all reactions addable enumerates minimal supports, i.e. elementary flux vectorstest_negative_cost_forces_reaction_into_every_design— a large negative cost forces a reaction in while the budget still bounds the resttest_intervention_costs_are_not_counted_twicetest_knockin_candidates_are_not_also_knockout_candidates12 of the 16 fail without the changes in this PR. The 4 that pass either way guard the fixes already merged in #79.
Validation
growth >= 0.001,max_cost 3): identical 353 designs, compared as sets--medium --large, incl.test_ecoli_core_mcs_455andtest_iml1515_mcs_393) run locally — reported below once complete🤖 Generated with Claude Code
https://claude.ai/code/session_01RFtof9nZXFvCNoXz19po8C