[Feature] way to add_boundary if not already present

[oxinabox]

Checklist

• [X] There are no similar issues or pull requests for this yet.

Problem

I want to add a boundary reaction if it is not present. Currently the easiest way to do this is to use model.add_boundary which will throw a ValueError if the reaction is already present as shown here then catch that ValueError and continue. This has a problem as add_boundary also throws a ValueError for a number of other error circumstances. So we are forced to catch the error and check it's message if we want to be robust against that, and this is awkward.

Solution

Three options:

• Make duplicate boundary addition throw a different exception type
• Make other error conditions not throw ValueError
• Add a bool argument that on duplicate reaction being added doesn't throw. throw_on_existing=false or some such

Alternatives

One can't just check if reaction is already in model.reactions before adding, as the automatic generation of reaction id is also in that function, here.

Anything else?

No response

[cdiener]

Hi, thanks for the request. I definitely see the point here. Just to clarify, by awkward you mean:

# ...
for m in mets:
    try:
        model.add_boundary(m)
    except ValueError as err:
        if not "already exists" in str(err):
            raise err
# ...

Correct? Maybe that is not too bad because it adds only one line compared to solution 1:

# ...
for m in mets:
    try:
        model.add_boundary(m)
    except AlreadyThereError:
        continue
# ...

Solutions

For (1) what would be the best error type? The other default ones do not really apply that well. We could create a custom error, but this would make catching it more awkward too, and we would need to use it consistently through the code base.

For (2) similar to (1).

For (3) it seems a bit overkill for a function that only adds a single reaction. If it would add many, that would make sense to me to skip the existing ones.

Alternatives

Agreed. In theory one could check for an identical Reaction.reactants with Model.reactions.query but that is clunky. Trying to add and catching the exception is definitely easier.

[oxinabox]

Indeed that is what I mean by awkward. Its not much code. But it does seem a bit fragile vs a type. E.g. if at some point someone decided to rephrase the exception text for some reason, I don't think they would expect it to break random code. It doesn't seem likely, so just "awkward". It feels like a code smell. But I am not really a python expert, so I could totes be off here.

[cdiener]

Fair enough, what would you prefer instead?

[Midnighter]

To be honest, I think that we should have many more custom errors and basically not raise any default type. However, that's a lot of work to clean up and also needs careful consideration in order to maintain backwards compatibility. So it's not likely to happen unless someone takes it on as a pet project.

[oxinabox]

Fair enough, what would you prefer instead?

Possibly best would be to mimic what PathLib does for creating directories. And have both an flag to surpess (exists_ok), and throw a custom exception type.

[cdiener]

Yeah, consistency would be the hard part here, especially for the custom errors. As long as we make it clear in the SemVer that backward compatibility should be okay. This is more on us for never switching to major versions.

It would be great if add_boundary would behave like all the other add_* methods, though. Currently, those either simply ignore existing objects (add_metabolites) or issue a warning in the log. I'm fine with either, (1) extra arg or (2) warning, but it should probably be the same for all the adders.

@oxinabox Did you want to take a stab at this in a PR?