Skip to content

fix: remove_redundant_facets mutates caller's lb/ub arrays (issue #83) - #137

Open
xovishnukosuri wants to merge 1 commit into
GeomScale:developfrom
xovishnukosuri:fix/remove-redundant-facets-no-mutation
Open

xovishnukosuri wants to merge 1 commit into
GeomScale:developfrom
xovishnukosuri:fix/remove-redundant-facets-no-mutation

Conversation

@xovishnukosuri

Copy link
Copy Markdown

Problem

remove_redundant_facets in dingo/pyoptinterface_based_impl.py modifies the lb and ub arrays it receives in-place. It replaces values with sys.float_info.max / -sys.float_info.max when marking redundant bounds and near-zero-width constraints.

PolytopeSampler.get_polytope() passes self._metabolic_network.lb and self._metabolic_network.ub directly into this function. After the first sampling run those arrays on the model are corrupted. Any subsequent call — a second generate_steady_states, an FBA, or an FVA on the same model — sees bounds like -1.79e+308 instead of the original values.

This is the root cause of the UnboundLocalError reported in issue #83: the second FBA on the corrupted model returns a non-OPTIMAL status, optimum_sol and optimum_value stay at their zero-initialised defaults, and downstream code that expects a real solution breaks.

Fix

Copy lb and ub at the top of remove_redundant_facets so all internal modifications are confined to local arrays. Two lines added, no API change, no performance concern for normal model sizes.

Test

Added test_remove_redundant_facets_no_mutation to tests/fba.py. It snapshots the model's lb and ub before the call and asserts they are byte-for-byte identical after it returns.

Reproduction (before fix)

import dingo

model = dingo.MetabolicNetwork.from_json("ext_data/e_coli_core.json")
model.set_opt_percentage(90)
sampler = dingo.PolytopeSampler(model)
sampler.generate_steady_states()

model.set_opt_percentage(20)
sampler = dingo.PolytopeSampler(model)
sampler.generate_steady_states()  # UnboundLocalError before fix

Closes #83

…mScale#83)

The function modified the lb and ub arrays in-place when marking
redundant facets and equality constraints. Any caller that passed
model.lb and model.ub (as PolytopeSampler.get_polytope does) would
find those arrays filled with sys.float_info.max/-sys.float_info.max
after the first call, causing FBA/FVA and subsequent sampling runs to
operate on corrupted bounds.

Fix: copy lb and ub at the top of the function so all internal
modifications are confined to local arrays.

Add a regression test that verifies the model's lb/ub are unchanged
after calling remove_redundant_facets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generate_steady_states fails when you edit a model's optimal percentage twice

1 participant