Fix knock-in pricing in the cost filter, and restore negative intervention costs - #79
Merged
Merged
Conversation
…udgets
Three defects on the same path. Each is invisible on its own, because a
knockout-only problem never touches any of them.
1. filter_sd_maxcost priced an intervention by dict membership, knockout dict
first, while SDProblem prices it knock-in over knockout. A reaction present
in both dicts was therefore charged its knockout cost after the MILP had
already priced it as a knock-in, and the design was discarded. The overlap is
the normal case rather than an edge case: compute_strain_designs defaults
ko_cost to every reaction whenever the caller passes only ki_cost.
This one is not specific to negative costs. With ko_cost={'R2': 50},
ki_cost={all: 1} and max_cost=3 on a four-reaction network, the MILP emits
{R1,R2,R4} at cost 3 and the filter re-prices it at 52, so the design is lost
silently. Gene-based problems escape it because ko_cost starts empty there.
Dispatch on the sign of the design value instead, which is the value
semantics the function already documents. Where ki_cost is empty the new
expression is identical to the old term for term, so knockout behaviour
cannot change.
The same expression zipped the filtered designs against the unfiltered cost
list when stamping '**cost**', misaligning labels and the resulting sort
whenever the filter removed anything. Fixed alongside, since correct stamping
is required once anything is filtered.
2. cont_MILP kept the three leading rows that constrain z alone. Stripped of
their z columns they read 0 <= max_cost, which is vacuous for a non-negative
budget but makes verify_sd reject every design once max_cost is negative.
Neutralise their right-hand sides in the continuous copy rather than dropping
the rows, whose positions z_map_constr_ineq columns are aligned to.
3. Row 0 asserted total cost >= 0. That is implied whenever costs are
non-negative, and z is binary so the objective is bounded without it. Its
only effect was to make negative costs infeasible by construction. Left open.
Together these restore negative intervention costs as a way of forcing a
reaction into every solution: give it a large negative cost and set max_cost so
that omitting it is unaffordable, while the budget still bounds the rest. On a
network with elementary flux vectors {R1,R2,R4} and {R1,R3,R4}, cost(R2)=-100
with max_cost=-97 now returns {R1,R2,R4} alone.
e_coli_core MCS (growth >= 0.001, max_cost 3) returns the identical 353 designs,
compared as sets. Test suite: 368 passed, 2 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <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.
Three defects on the same path. Each is invisible alone, because a knockout-only problem never touches any of them. The first causes silently wrong results with ordinary positive costs and is independent of the other two.
1.
filter_sd_maxcostcharged a knock-in its knockout costnetworktools.filter_sd_maxcostpriced an intervention by dict membership,kocostfirst:SDProblemprices it the other way round — knock-in wins over knockout (strainDesignProblem.py:147,149-151, whereko_cost[i]is nan-masked whereverki_cost[i]is set). A reaction present in both dicts is therefore re-priced after the fact and the design is discarded.The overlap is the normal case, not an edge case:
compute_strain_designs.py:476-477defaultsuncmp_ko_costto every reaction id whenever the caller passes onlyki_cost.This is not specific to negative costs. On a four-reaction network with elementary flux vectors
{R1,R2,R4}and{R1,R3,R4}, usingko_cost={'R2': 50},ki_cost={all: 1},max_cost=3, all positive:[['R1','R3','R4']]The MILP finds
{R1,R2,R4}at cost 3; the filter re-prices it at 52 and drops it. Gene-based problems escape this becauseuncmp_ko_coststarts empty there, which is why no existing test catches it.Fixed by dispatching on the sign of the design value (
+1knock-in,−1knockout), the value semantics the function already documents. Whereki_costis empty the new expression is identical to the old term for term, so knockout behaviour cannot change by construction.The same expression also zipped the filtered designs against the unfiltered cost list when stamping
'**cost**', misaligning the labels and the resulting sort whenever the filter removed anything. Fixed alongside, since correct stamping is required once anything is filtered.2.
cont_MILPmadeverify_sdreject everything at a negative budgetcont_MILPkeeps the three leading rows that constrainzalone. Stripped of theirzcolumns they read0 <= max_cost— vacuous for a non-negative budget, but false for a negative one, so every design failed verification. Their right-hand sides are now neutralised in the continuous copy. The rows are kept rather than dropped, becausez_map_constr_ineqcolumns are aligned to row positions.3. Row 0 forbade a negative total cost
Row 0 asserted
total cost >= 0. That is implied whenever costs are non-negative, andzis binary so the objective is bounded without it. Its only effect was to make negative costs infeasible by construction. Left open; the row itself is kept so row indices stay put.What this restores
Negative intervention costs as a way of forcing a reaction into every solution — give it a large negative cost and set
max_costso that omitting it is unaffordable, while the budget still bounds the rest:cost(R2)max_cost{R1,R2,R4}only — R2 forced, ≤3 other units{R1,R2,R4}only{R1,R2,R4}only, unbounded restValidation
growth >= 0.001,max_cost 3): identical 353 designs, sizes {1:19, 2:111, 3:223}, compared as sets of frozensets — 0 missing, 0 extramainNot addressed, flagged deliberately
compute_strain_designs.py:476-477defaultingko_costto all reactions. Fixing it there would shrink the MILP's knockout candidate set and change ordinary behaviour, so it is left alone.strainDesignSolutions.py:219-243resolves the same overlap by summing both dicts, so a reaction in both is double-counted in reported costs. Reporting only; does not affect design sets.🤖 Generated with Claude Code
https://claude.ai/code/session_01RFtof9nZXFvCNoXz19po8C