fix: bound assignment for reverse exchange rxns

[BenjaSanchez]

There is a problem when assigning uptake/secretion rates on exchange reactions that come in the non-standard direction --> X: The way that we handle them at the moment is just multiplying lower/upper bound by -1, e.g. if uptake rate is to be assigned = -3:

lower_bound, upper_bound = -1 * (-3), -1 * (-3) = +3, +3

Similarly if secretion rate is to be assigned = +3:

lower_bound, upper_bound = -1 * (+3), -1 * (+3) = -3, -3

However this breaks if we account for uncertainty of let’s say 0.3 in the uptake:

lower_bound, upper_bound = -1 * (-3.3), -1 * (-2.7) = +3.3, +2.7  # inconsistent, LB > UB

And also in the secretion:

lower_bound, upper_bound = -1 * (+2.7), -1 * (+3.3) = -2.7, -3.3  # also inconsistent, LB > UB

The solution is to swap LB for UB, as done here. It is also tested on eciML1515, which has non-standard directions for any uptake reaction. AFAICT there is no other part in the simulations service that uses this logic :)

[codecov-io]

Codecov Report

Merging #150 into devel will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##            devel     #150   +/-   ##
=======================================
  Coverage   74.83%   74.83%           
=======================================
  Files          20       20           
  Lines         906      906           
=======================================
  Hits          678      678           
  Misses        228      228

Impacted Files	Coverage Δ
src/simulations/modeling/adapter.py	65.92% <100%> (ø)	:arrow_up:

---

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 096eece...dcebe44. Read the comment docs.

[BenjaSanchez]

@kvikshaug to be fair I only caught it bc I was testing a model with "non-standard" exchange reactions. Also, not sure how many users use the uncertainty feature...

[Midnighter]

The PR itself could, of course, be perfected by including unit tests that will ensure these cases are covered in future :wink:

[BenjaSanchez]

@Midnighter actually it is now tested, as eciML1515 has non-standard exchange reactions and I'm adding uncertainty to an uptake rate (which was failing before this change). Or did you had any other tests in mind?

[Midnighter]

I meant adding a unit test, for example to https://github.com/DD-DeCaF/simulations/blob/devel/tests/unit/test_simulations.py, that will be run all the time. Unless I missed it, working with eciML1515 is something you did manually so it is easily forgotten over time.

[BenjaSanchez]

@Midnighter eciML1515 was added in PR https://github.com/DD-DeCaF/simulations/pull/147 to the simulations service as a test model, and it is now routinely used for unit tests. When I said that the non-standard case this branch covers "is now tested", I meant that the unit test test_measurements_adapter_ec_model now includes testing uncertainty in the glucose uptake rate of eciML1515, as you can see here (uptakes in eciML1515 have the non-standard exchange reaction formalism).