-
Notifications
You must be signed in to change notification settings - Fork 217
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
Context Managers #306
Comments
In your example, the
could you elaborate why this is much more boilerplate? The methods will anyway somehow have to define how the changes are to be undone no? |
Its not much more boilerplate, thats fair. I just don't necessarily like the idea of carrying the time machine around with you if you don't have to (rather than having the model take care of that). It seems like one more API to learn, another kwarg to specify for each function. In my example, functions like knock_out can go on modifying with model:
model.reaction.my_favorite_reaction.bounds = (0,0) And not have to worry about specifying their own undo function. We will have to define how to undo operations, that's true. But if the context manager only has to worry about a few use cases, these might get better implemented in the property methods themselves. (i.e., model.add_reactions could check for a context manager), rather than implementing do/undo functions in something |
I think the alternative with less boiler plate would be the copy mechanism where you copy the model once and apply the changes to the copy. This is how it is done in cobrapy now and in many packages depending on cobrapy. This has the advantage that it does not require implementing undo functions for everything. A history context obviously has more detailed control since it allows reversing individual steps within the context. However, looking at cameo |
There's just not much reason for implementing a context manager if its just going to call model.copy. Then we're just doing ko_model = model.copy()
ko_model.reactions.my_favorite_rxn.knock_out()
ko_model.optimize() as usual, and we don't really need the context... Whats the ProblemCache example? I would assume that the use of context managers would be to allow us to make temporary changes to a model without affecting its top-level scope. So we could do something like this to check reaction essentiality: for reaction in model.reactions:
with model:
reaction.knock_out()
model.optimize() |
Exactly, model.copy does already resolve a lot of the problems already. I agree that this would not be efficient in a loop. Is that used a lot? Never seen that in cobrapy or other people's code? If we implement context managers I would also prefer a few for the most important attributes like bounds and temporarily adding reactions. |
Copying models is too slow. That's why we started using the TimeMachine pattern in cameo. The reason that you don't see this a lot in the cobrapy codebase is that most functions operate on the solver object directly (which is even more boilerplate code). @pstjohn getting rid of additional kwargs sounds attractive. Why not something like
since |
Yeah I like that pattern. I noticed you're putting in hooks to |
Yeah, I agree. For simple setters we could get around some boilerplate code with an "undoable" decorator. Based on @phantomas1234's example: def undoable(f):
def wrapper(self, value):
if hasattr(self, "context"):
context = self.context
elif hasattr(self, "model") and hasattr(self.model, "context"):
context = self.model.context
else:
context = None
if context:
old = getattr(self, f.__name__)
context(do=partial(f, self, value), undo=partial(f, self, old))
else:
f(self, value)
return wrapper This is assuming that the setter method has the same name as the property, otherwise you could make the decorator take this as argument. The result would be: @lower_bound.setter
@undoable
def lower_bound(self, new_bound):
self._lower_bound = new_bound |
So before I do any of this it might make sense to query the team. One role context managers will likely have to play is in managing the temporary addition and removal reactions and metabolites to models. I'm thinking this would be easiest if we standardized the different ways objects combine in the base classes. For instance, Model.py implements My suggestion would be to move all these functions to a
and then have a master dictionary/class which, given two class names, returns the correct function. I'd then implement I haven't thought through all the details, for instance
But in general, we would keep the existing functions in place. def remove_from_model(self, remove_orphans=False):
self._model -= self
if remove_orphans:
...find_orphans...
for orphan in orphans:
self._model -= orphan and context management would get handled under the hood by the lower-level |
Still being rather new to the field - is repeated adding / removing or reactions and metabolites a common task? Could you mention a couple of important use-cases you have in mind that this will enable? Reason I'm wondering is that particularly given the interaction with the solver object I'm wondering if this will not be tricky to get right and if so, it is important that it is of high priority enough for the delay it could entail for porting cameo algorithms to cobrapy, and the move to devel-2 as mainstream. Of course, the model as context manager approach will give the user the feeling that anything that is done to the model will be rewound upon exit, but this I think anyway is an overly ambitious goal anyway. Raising the question how to make it clear what is resettable and not since the decorator is not visible.. Throwing error on non-supported object changes while in context could maybe be one approach. I think the most logical definition of Model.add_reaction is in Model and not some other module.. The idea of having a dictionary to keep track of the add/remove attributes between model, gene, metabolite, reaction sounds creative but also a bit confusing although I think it mainly highlights the, perhaps necessary, graphy relationship between these classes. Could we imagine a scenario where we introduce the CM a bit more stepwise? Supporting the simple attributes first and then introduce more features as we go forward? |
I never had any problems with the API as it is now. I understand you motivation with the def remove_from_solver(metabolites=None, reactions=None):
...
def add_to_solver(metabolites=None, reactions=None):
... would help to to the heavy lifting like connecting to optlang and manage the history (both are the undos of each other). At least that way you only call one additional function in all of the |
@hredestig I think repeated adding and removing of reactions and metabolites is uncommon. There certainly needs to be a way to add and remove but I don't see that as something that needs to be handled by a context manager. @pstjohn From an object-oriented perspective it seems odd to move the actions that operate on an object to another place. I worry that in the future we'll have trouble maintaining the code if the code for an object is scattered in multiple places. |
Good points, thanks for the comments! I'll offer a couple of counter-arguments. On adding / removing reactions, you could imagine an for metabolite in model.metabolites:
with model:
sink = create_sink_rxn(metabolite)
model.add_reaction(sink)
model.objective = sink
model.optimize() For metabolites, you could imagine scanning over various P/O ratios, for instance. On object-oriented code, I'd argue its cleaner to inherit the methods from I'm getting a bit off-topic here, but ultimately everything in cobrapy will operate on a model or reaction. You could make similar arguments for implementing the entirety of |
I think regarding the context manager I agree that adding and removing reactions should be captured. Not so much because there are many examples but more form a usability stand point: its much easier to say that all operations are reversed than to have a list of a few that will and others that won't. Sorry, I didn't understand your description before. You mean to add by reference and not by ID right? That makes more sense :) I think it's unusual to implement behavior of child classes in the parent class, but I don't think it's necessarily bad. Why is |
On |
I agree. That is weird.
…On Dec 2, 2016 16:14, "Peter St. John" ***@***.***> wrote:
On remove_reaction, its in Model, but its just a thin wrapper around
reaction.remove_from_model or reaction.delete. My point being that the
methods are already somewhat fragmented.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#306 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG0pD3-SghEmuzixQX92mzmA7Rb1JNaCks5rEJhAgaJpZM4KmR-G>
.
|
Also, on the API: I wasn't planning to change it, this was more a suggestion on a way to standardize things at a lower level to make it easier to implement the CM undo's. |
Yep that sounds reasonable. |
#312 merged, let's open new issues and refer to this one if we want to continue the discussion |
This is to split off discussion on context managers specifically from #303.
One option is to adapt
TimeMachine
from cameo directly. I would argue this adds a lot of boilerplate code to common functions, but it would definitely make porting other code from cameo more straightforward.It seems like TimeMachine is used in cameo for track:
model.solution.x_dict
)Are there other uses I'm missing?
My suggestion would probably be to add a similar mechanism to
cobra.Model
, wheremodel.{{ history }}
(name suggestions?_context
?_history
?) would keep track of changes to a model's state since it entered the context. We could then try to implement property methods at as low a level as possible to update the context objects accordingly.Here's some pseudocode for I could imagine this working:
This is super simplified, we'd definitely want to make an object that would keep track of different types of changes, the order in which they were applied, etc. But with something like this, we could initialize a context with something like
in which everything inside the context would hopefully be undone by the
__exit__
method...The other option could be to create nested contexts explicitly with functions like
model.with_media
,model.with_reaction_knocked_out
, etc, where we could chain them together as per @cdiener 's suggestion.The text was updated successfully, but these errors were encountered: