Fix FastCC #1428
Fix FastCC #1428
Conversation
There was a problem hiding this comment.
I mostly checked for style etc. Perhaps @dagl1 could look at the functional side, I'm not into the matter at the moment.
|
|
||
| for rxn in rxns: | ||
| var = prob.Variable( | ||
| "auxiliary_{}".format(rxn.id), lb=0.0, ub=flux_threshold |
There was a problem hiding this comment.
| "auxiliary_{}".format(rxn.id), lb=0.0, ub=flux_threshold | |
| f"auxiliary_{rxn.id}", lb=0.0, ub=flux_threshold |
|
|
||
| # Enable constraints for the reactions | ||
| for rid in rxn_ids: | ||
| model.constraints.get("aux_constraint_{}".format(rid)).lb = 0.0 |
There was a problem hiding this comment.
| model.constraints.get("aux_constraint_{}".format(rid)).lb = 0.0 | |
| model.constraints.get(f"aux_constraint_{rid}").lb = 0.0 |
| for rid in rxn_ids: | ||
| model.constraints.get("aux_constraint_{}".format(rid)).lb = 0.0 | ||
|
|
||
| obj_vars = [model.variables.get("auxiliary_{}".format(rid)) for rid in rxn_ids] |
There was a problem hiding this comment.
| obj_vars = [model.variables.get("auxiliary_{}".format(rid)) for rid in rxn_ids] | |
| obj_vars = [model.variables.get(f"auxiliary_{rid}") for rid in rxn_ids] |
| const = model.constraints.get("constraint_{}".format(rxn.id)) | ||
| var = model.variables.get("auxiliary_{}".format(rxn.id)) | ||
| for rxn in rxn_ids: | ||
| const = model.constraints.get("aux_constraint_{}".format(rxn)) |
There was a problem hiding this comment.
| const = model.constraints.get("aux_constraint_{}".format(rxn)) | |
| const = model.constraints.get(f"aux_constraint_{rxn}") |
| var = model.variables.get("auxiliary_{}".format(rxn.id)) | ||
| for rxn in rxn_ids: | ||
| const = model.constraints.get("aux_constraint_{}".format(rxn)) | ||
| var = model.variables.get("auxiliary_{}".format(rxn)) |
There was a problem hiding this comment.
| var = model.variables.get("auxiliary_{}".format(rxn)) | |
| var = model.variables.get(f"auxiliary_{rxn}") |
|
@Midnighter Ah sorry I didn't realise things required checking. Some small things in regards to the code (but both of you are my seniors so take them as questions) |
| result = [] | ||
| # Disable constraints for the reactions | ||
| for rid in rxn_ids: | ||
| model.constraints.get("aux_constraint_{}".format(rid)).lb = -LARGE_VALUE |
There was a problem hiding this comment.
Optlang includes removal of constraints, as far as I can see, keeping these for later use isn't necessary?
There was a problem hiding this comment.
You could add and remove them in every iteration but this is much slower. Adding constraints means extending the constraint matrix and removing contracting it. Changing a bound however is a constant time operation and very quick.
| @@ -10,9 +11,44 @@ | |||
| if TYPE_CHECKING: | |||
| from cobra.core import Model, Reaction | |||
|
|
|||
| logger = getLogger(__name__) | |||
| LARGE_VALUE = 1.0e6 | |||
There was a problem hiding this comment.
Floating constant; in the matlab implementation this is derived from the solver tolerance (which I don't like that much either), would there be anything against directly adding this as optional argument for the functions?
There was a problem hiding this comment.
Note that this is not the tolerance (it's 1e6 not 1e-6). Do deactivate the constraints I just want to open its bound, but optlang does not allow unbounded constraints so I just set a very large (in absolute value) lower bound here to turn off the constraint.
|
Thank you for your evaluation @dagl1.
It's not so much that it's strictly required but since most of us work alone rather than pair program, it's always better to have a second pair of eyes. |
This brings the method in line with the paper. Still not super fast but it works. Still missing some more tests.