Set correct flux bounds

[MaxGreil]

This PR is supposed to fix #858.

[codecov-io]

Codecov Report

Merging #866 into devel will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            devel     #866      +/-   ##
==========================================
- Coverage   84.39%   84.38%   -0.02%     
==========================================
  Files          47       47              
  Lines        4216     4213       -3     
  Branches      981      981              
==========================================
- Hits         3558     3555       -3     
  Misses        424      424              
  Partials      234      234

Impacted Files	Coverage Δ
cobra/core/reaction.py	87.77% <100%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 374ebfe...877339a. Read the comment docs.

[matthiaskoenig]

Hi @MaxGreil,
this looks good.

Some smaller things I would change

1. use CONFIGURATION.lower_bound directly instead of defining local variables for them.
2. set upper and lower bounds at the same time (not part of issue, but if you touch the code you should update this)

The problem with setting the upper and lower bounds individually, is that there are 2 checks performed (both time it is checked if the bounds fit, whereas setting both at once requires only one check). In addition as far as I remember the constraints in optlang are generated at once, instead after each other (which results in some speedup for many reactions).

I.e. use

    @bounds.setter
    @resettable
    def bounds(self, value):
        lower, upper = value
        # Validate bounds before setting them.
        self._check_bounds(lower, upper)
        self._lower_bound = lower
        self._upper_bound = upper
        self.update_variable_bounds()

to set the upper and lower bound at the same time directly from configuration, i.e. instead of

self.lower_bound = lower_bound
self.upper_bound = upper_bound

better

self.bounds(CONFIGURATION.lower_bound, CONFIGURATION.upper_bound)

Same at the other two locations.

[cdiener]

Looks good. Would be helpful to add a test to avoid regressions in the future.

[MaxGreil]

Hi @cdiener ,
how should this test look like?

[cdiener]

Hi @cdiener ,
how should this test look like?

Sets default bounds via configuration, adds a reaction with build_reaction_from_string and checks that the set bounds were applied. Could be added below

cobrapy/cobra/test/test_core/test_core_reaction.py

Line 238 in 374ebfe

def test_build_from_string(model):

.

[Midnighter]

Sorry, didn't get a chance to look at this earlier. Just some style changes:

• There is no need for parentheses in these cases.
• Please rename the global CONFIGURATION variable to config. I would like to do this everywhere anyhow.

[matthiaskoenig]

@MaxGreil you are almost there. Just write the test and do the minor changes proposed by @Midnighter then this pull request can be merged.

[Midnighter]

Thanks a lot for your contribution @MaxGreil 😃