cobrapy
cobrapy copied to clipboard
opencobra
Deprecate the lower and upper bound behavior
Please voice your opinion here and state your use-case for the old behavior of setting bounds, i.e., constraining a reaction to a single flux value when setting the lower bound to be larger than the upper bound or when setting the upper bound to be smaller than the lower bound.
Maybe to give some context, this behavior was introduced when reaction.bounds did not exist yet. Furthermore, in the pre-optlang days of cobrapy, it was always possible to set the lower bound higher than the upper bound. Only during a simulation would the solver interface choke on that eventually (I hope I remember that correctly). So, while implementing the new solver interface (in cameo at that point) I saw two options.
- Stick to what optlang does: if the lower bound exceeds the upper bound (and the other way around) an exception will be raised forcing the user to look before leaping by writing conditionals to figure out which bound to change first (if both bounds need to be changed).
- Implement the current behavior: see Mortiz's comment above and also the figure below for my interpretation of why the current behavior makes sense (at least to me, I can see how different people might expect a different behavior when no exception is raised).
I went for No. 2 because setting both lower and upper bounds for reactions is a common task and I wanted to make it convenient. I started using cobrapy and cameo for teaching and it was important to me to keep it as easy to use as possible. Also, the third option (and best solution) reaction.bounds might have crossed my mind but I didn't want to deviate too much from the cobrapy API (I could have easily opened a PR though ...).
So, I apologize for this dubious design decision 🙁. I am not sure anyone has ever been bitten by this and deprecating this behavior will break code. On the other hand I think it the right thing to do and all problematic code should be fixed with the use of reaction.bounds.
So, I apologize for this dubious design decision. I am not sure anyone has ever been bitten by this and deprecating this behavior will break code. On the other hand I think it the right thing to do and all problematic code should be fixed with the use of
reaction.bounds.
I actually think this behavior is okay. Not ideal but totally fine. The problem I see with it (and why I put the deprecation warning on it that maybe should have been a user warning) is that it is some extra behavior in a property, i.e., people will use it just like an attribute and never look at the documentation for it. It is also not to be found in the documentation.
So this warning is my attempt at seeing how many people have used this behavior without knowing about it and have fixed their reactions unwittingly. I'm not 100% committed to this path and I am open to removing the deprecation warning again after gaging impact. Especially now that everything will become better documented with Greg's PR #792.
This behaviour has now been removed in #1058 and is superseded by the bounds attribute. In all this time, nobody ever complained here so I guess it's rarely used, if at all. @phantomas1234 do you have someone who could look through the cameo codebase before the next cobrapy release? I would guess that it might get used there?
Can we convert this issue into a discussion so that in future, users can refer to the points presented here? Also, it becomes a bit of a problem to scavenge through closed issues.
I am out of this, does this mean there is no reaction.lower_bound anymore and one always has to specify both bounds? In any case, @carrascomj can you please evaluate the consequences of this breaking change for cameo?
From what I can see in the PR, it just means that, while reaction.lower_bound and reaction.upper_bound can be used, if the user inputs a wrong value for any of the 2, the other bound won't be corrected to match the input (fixing the flux). I will check if cameo used this functionality in that way.
