Adding a reaction that is already part of a model to another model creates an alias (not a copy)

[oxinabox]

Consider:

model1 = cobra.Model("M1")
model1.add_reactions([cobra.Reaction("R1")])
model2 = cobra.Model("M2")
model2.add_reactions(model1.reactions)

print(model1.reactions.R1.bounds)
model2.reactions.R1.bounds = (500,600)
print(model1.reactions.R1.bounds)

which prints:

(0.0, 1000.0)
(500, 600)

We would have expected that changing a reaction on model2 would have no effect on model1.
But actually those reactions are the same.
In fact:

print(model1.reactions.R1.model is model2)

prints True.

Probably add_reactions should always create a copy if reaction.model is set?

Related: #673

[oxinabox]

ran into this again today.
A more complex MWE

import cobra 

m1 = cobra.Model("m1")
r_out = cobra.Reaction("r_out")
m1.add_reactions([r_out])
r_out.build_reaction_from_string("A --> nothing")

r_in1 = cobra.Reaction("r_in1", lower_bound=-10, upper_bound=10)
m1.add_reactions([r_in1])
r_in1.build_reaction_from_string("nothing --> A")

m1.objective = r_out

m1.slim_optimize()
# as expected outputs 1000

###################

m2 = cobra.Model("m2")
r_out2 = cobra.Reaction("r_out2")
m2.add_reactions([r_out2])
r_out2.build_reaction_from_string("2 A --> nothing")
m2.objective = r_out2

# Add a reaction from m1
m2.add_reactions([m1.reactions.r_in1])

m2.slim_optimize()  # as expected outputs 500

########
# Now go back to working on original model

m1.reactions.r_in1.bounds = (-7, 7)
m1.slim_optimize()  # Unexpected: outputs 1000 still!

m2.slim_optimize()  # Unexpected outputs 3.5

[cdiener]

Hi, I agree that this creates issues for sure. The copy approach sounds good.

One complicated edge case I could imagine is using context manager and remove reactions. This will unset the model attribute on the reaction but reset it after exiting the context. That could potentially lead to a situation where that bug is still triggered when the reaction is added to another model within the context.

Another question is whether we should issue a warning in those cases.

[oxinabox]

One complicated edge case I could imagine is using context manager and remove reactions.

I see.
This is pretty niche.
Since one doesn't normally temporaril remove reactions from a model.
Instead one normally just bounds them to zero.

And if programatically reworking a model one wouldn't normally be using a context manager at all