fix/refactor add/remove genes,metabolites,reactions

[hredestig]

breakout from discussion in #311
these should allow resetting using history manager

[pstjohn]

Where do we want to the code for adding metabolites / reactions to a model to live?
we should standardize these methods, and perhaps deprecate/remove ones we feel are unnecessary.

• model.add_reactions() vs. reaction.add_to_model() (doesn't exist currently)
• model.remove_reactions() vs. reaction.remove_from_model() vs. reaction.delete()

and then the same with metabolites (the functions should be identical).

We'll have to be careful about how we handle temporary deletions... We need to be able to keep the object intact somewhere so we can add it back to the model at the end of the context.

Also important will be the Reaction.add_metabolites, which should get context'ed.

[hredestig]

I would have put the implementations in the classes they belong to (organise code by class, or by function..) but that's just cosmetics and won't impact functionality / user experience so don't have a strong opinion on that. I find the cobrapy classes already a bit tricky to understand and I believe the right methods help here. So, model.add_reactions makes perfect sense but reaction.add_to_model would go against the 'hierarchy' and therefore better avoided. In my mind, 'remove' and 'delete' is somewhat synonymous so better keep only reaction.remove_from_model and add the delete functionality to that function. delete does perhaps not need to be undoable though.

[pstjohn]

I guess my point is that we should take the opportunity to standardize what the methods are called and where they live. I.e., why have reaction.remove_from_model but not add_to_model? Why have Model.remove_reactions but not remove_metabolites?

On the code, I'm more saying should the actual mechanics be implemented in Reaction (and Metabolite) or Model? Right now add_reactions does most of the heavy lifting in Model.py, while remove_reactions is just a thin wrapper around Reaction.remove_from_model... So I think there's some room for rethinking about how we want these methods to behave.

[Midnighter]

I'm all for standardizing method names and I do believe your second paragraph is important. To me, the hierarchy of what classes represent also implies which methods they should have.

• Metabolites are pretty much atomic units.
• Reactions are primarily expressions/equations involving metabolites plus annotation.
• Model is primarily a bunch of reactions.

That means, IMHO, that reactions need methods to add/remove metabolites from themselves and models need methods to add/remove reactions from themselves. Nothing more, nothing less.

There is one interesting exception, of course. It is useful to get the set of all metabolites that occur in all reactions of a model. If we want that attribute model.metabolites and as per above reasoning Model does not immediately know about metabolite changes to reactions, there are two ways of dealing with that: Either that property is always computed on the fly (even for large models that shouldn't be too costly) or we consider reactions as immutable, i.e., if you want to change the metabolites partaking in a reaction that is essentially a new reaction (remove the old one and add the new reaction to the model).

[hredestig]

I was thinking a bit more in the line of incremental changes and then the motivation for reaction.remove_from_model 's existence is that it's there now. In the longer run, perhaps already for 0.6 if you like, then I agree with Moritz, models handle the adding/removing reactions from itself.

[pstjohn]

@Midnighter, the class hierarchy was something I thought about a good about when doing the context managers. If objects only looked down the stack (Model -> Reaction -> Metabolite), then a metabolite or reaction object could easily belong to more than one model, and model copying would become much faster (since the underlying objects wouldn't change).

The problem is that a lot of the current methods rely on the fact that objects can look up the stack: (Metabolite.reactions, for instance). So that's a fairly major change in how these things are implemented, sadly. But its worthing keeping in mind if we ever want to go that route. We could drop Model.repair, for instance, and calls to Metabolite.reactions would get replaced by method calls form Model.py. There might be some concerns on speed (calculating set intersections on the fly vs. when reactions are added), but we could probably get around this with some clever caching of results.

---

More to the current issue:

I do agree on backwards compatibility, but there's enough of an API change in 0.6 that I think we can safely refactor these methods if we want to. My main goal is to make things consistent, so whether that's deprecating Reaction.remove_from_model vs. adding Reaction.add_to_model; deprecating Model.remove_reactions vs. adding Model.remove_metabolites, I'm not really too particular.

[hredestig]

The consistency is really nice to have but there is also the usefulness aspect, I'm sure you have a better idea than me on what's meaningful to have, so if Model.remove_metabolites is useful then I vote we add it, from my perspective though I couldn't immediately think of clear situations when I'd need that.. I think we should avoid adding things only for the sake consistency.

[Midnighter]

The problem is that a lot of the current methods rely on the fact that objects can look up the stack: (Metabolite.reactions, for instance). So that's a fairly major change in how these things are implemented, sadly. But its worthing keeping in mind if we ever want to go that route. We could drop Model.repair, for instance, and calls to Metabolite.reactions would get replaced by method calls form Model.py. There might be some concerns on speed (calculating set intersections on the fly vs. when reactions are added), but we could probably get around this with some clever caching of results.

I agree it's a major change and maybe more suitable for something close to 1.0 ;) From my limited experience with the cobra codebase what you describe is a major pain point for maintenance. All objects are extremely entangled meaning that refactoring affects large parts of the code base. And even worse, I've seen a bunch of code that accesses "private" attributes of composite objects, e.g., self.model._private_attribute.modify. That should not happen.

I do agree on backwards compatibility, but there's enough of an API change in 0.6 that I think we can safely refactor these methods if we want to. My main goal is to make things consistent, so if that's whether thats deprecating Reaction.remove_from_model vs. adding Reaction.add_to_model; deprecating Model.remove_reactions vs. adding Model.remove_metabolites, I'm not really too particular.

My vote then is to make the necessary changes on the Model class in order to move it more in direction of our above discussion.

[pstjohn]

if Model.remove_metabolites is useful

I'm suggesting that we would replace the implementation currently in Metabolite.remove_from_model with a thin wrapper around Model.remove_metabolites([self]), which would get context-managed.

Same with Reaction.remove_from_model, etc.

Otherwise, we context manage the code in Reaction.remove_from_model, and Model.remove_reactions remains a thin wrapper around that method: the question is really what method we want to hold the code.

@Midnighter, we'll also have to totally rethink things like Reaction.objective_coefficient, Reaction.flux, Metabolite.summary... Lets split this off into a separate issue.

[hredestig]

Your suggestion sounds good to me -> Model holds the code

[hredestig]

Closed with #404 discussion continues in #394