Skip to content

fix: sample_from_fva_output crashes with NameError on every call - #138

Open
xovishnukosuri wants to merge 1 commit into
GeomScale:developfrom
xovishnukosuri:fix/sample-from-fva-output-staticmethod
Open

xovishnukosuri wants to merge 1 commit into
GeomScale:developfrom
xovishnukosuri:fix/sample-from-fva-output-staticmethod

Conversation

@xovishnukosuri

Copy link
Copy Markdown

Summary

PolytopeSampler.sample_from_fva_output is a @staticmethod but contained two bugs that made it crash with NameError on every call:

  • It referenced self._parameters["tol"] (lines 294-295). There is no self in a static method.
  • It called get_matrices_of_low_dim_polytope(S, min_fluxes, max_fluxes, opt_percentage, tol) where tol was also undefined, and the argument order did not match the function's signature (S, lb, ub, min_fluxes, max_fluxes).

Fix

  • Define tol = 1e-06 locally, matching the default in PolytopeSampler.__init__.
  • Pass min_fluxes and max_fluxes as the lb/ub arguments to get_matrices_of_low_dim_polytope. When FVA output is the input, the FVA bounds serve as both the flux bounds and the tight bounds for equality detection — the same pattern used in get_polytope().

Reproducer

from dingo import MetabolicNetwork, PolytopeSampler

model = MetabolicNetwork.from_json("ext_data/e_coli_core.json")
min_fluxes, max_fluxes, flux_vector, objective = model.fva()

# Before this fix: NameError: name 'tol' is not defined
steady_states = PolytopeSampler.sample_from_fva_output(
    min_fluxes, max_fluxes,
    model.objective_function, objective,
    model.S,
)

Test plan

  • Run python -m pytest tests/fba.py tests/sampling.py to confirm no regressions
  • Call PolytopeSampler.sample_from_fva_output with e_coli_core FVA output and confirm it no longer raises NameError

🤖 Generated with Claude Code

PolytopeSampler.sample_from_fva_output is a @staticmethod but
referenced self._parameters["tol"] (lines 294-295), which raises
NameError immediately. It also passed the wrong positional arguments
to get_matrices_of_low_dim_polytope: the call used opt_percentage and
the undefined name tol where min_fluxes and max_fluxes are expected.

Replace self._parameters["tol"] with the literal 1e-06 (matching the
default set in __init__) and fix the argument order so that min_fluxes
and max_fluxes are passed as the lb/ub and fva-bound arguments that
get_matrices_of_low_dim_polytope expects.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

1 participant