Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] property overload Reaction.lower_bounds/upper_bounds #1403

Open
1 task done
oxinabox opened this issue Sep 2, 2024 · 1 comment
Open
1 task done

[Feature] property overload Reaction.lower_bounds/upper_bounds #1403

oxinabox opened this issue Sep 2, 2024 · 1 comment

Comments

@oxinabox
Copy link
Contributor

oxinabox commented Sep 2, 2024

Checklist

Problem

I want it to be impossible to set a Reaction's lower_bound and upper_bound inconsistently.

That is to say if the lower-bound is ever set to above the current upper-bound I want the upper-bound to also be updated, and visa versa

Solution

If we user property setter overloads we could accomplish this by overloading what happens when you set either of them.

Alternatives

This is the code am currently using

def set_lower_bound(reaction: Reaction, lower_bound: float) -> None:
    if reaction.upper_bound < lower_bound:
        reaction.upper_bound = lower_bound
    reaction.lower_bound = lower_bound


def set_upper_bound(reaction: Reaction, upper_bound: float) -> None:
    if reaction.lower_bound > upper_bound:
        reaction.lower_bound = upper_bound
    reaction.upper_bound = upper_bound

its fine and maybe its the most sensible

Anything else?

This is just an idea i had when I noticed that we were writing code like the above.

But using property overloads like this is pretty magic.
It might be too unexpected that changing one thing could result in multiple things changing.

@cdiener
Copy link
Member

cdiener commented Sep 2, 2024

Hi, thanks for the comment. Lower and upper bounds are already properties and have (overloaded) setters.

The behavior you have in your code used to be the default. But this behavior was deprecated after discussion in favor of using Reaction.bounds = lb, ub which is a bit more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants