-
Notifications
You must be signed in to change notification settings - Fork 38
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
Implement hooks with register functions/decorators #957
base: master
Are you sure you want to change the base?
Conversation
On thing I like about this approach is that it gets rid of the if statement to check for the appropriate function whenever a hook is called. With this approach it is just a single line calling the hook execution. |
Thank you for taking a stab at this, @jgosmann !
Are you saying that the syntax for doing hooks in IPythonViz will be different than the syntax for doing it in Nengo GUI? What happens if someone accidentally uses the wrong syntax? (Or should I wait on questions like this until it's actually implemented? ) |
No the syntax is exactly the same (I should have given an example): from nengo_gui import hooks
@hooks.on_step
def do_something(sim):
# ... When run outside of the GUI this registers a global hook, when executed by the GUI (e.g. code type into the internal editor) it will register a page-local hook. (Actually, registering a global hook from inside the GUI is sort of complicated.) I think this should give the behaviour that one would expect. The hooks will be shared across multiple InlineGUI instances, but those live all in the same IPython kernel anyways (so it makes sense). In the InlineGUI one also cannot open multiple pages (I believe, at least it is somewhat complicated and not intended). |
I've just submitted a PR with a slightly different version of this #965 . The main thing I was trying to change from this version is the global registration approach used when running in Jupyter notebook. With the version in this PR, if you have two cells with two different nengo_gui models, or if you re-execute the cell that defines the hooks, then all of those hooks will stay registered in the global registration list. As an example, consider this notebook: import nengo
import nengo_gui
model = nengo.Network()
@nengo_gui.hooks.on_step
def on_step(sim):
print('step %g' % sim.time)
import nengo_gui.ipython
nengo_gui.ipython.IPythonViz(model) If you re-execute that cell, each time you re-execute it you'll add yet another print statement to the global |
I might argue that this is actually how it should behave. |
Hmm, interesting.... my intuition goes in completely the opposite direction. When would you want that behaviour? Why would you want it to be executing code that no longer exists? What if you change that function? That means it'll now execute both the new version of the function and the old version. What if there's a syntax error the first time you write the function? Now you can't fix it without restarting the kernel. |
Also, how would you handle the case of having two different cells using two different hooks within the same notebook? |
The code @nengo_gui.hooks.on_step
def on_step(sim):
print('step %g' % sim.time) says to register a hook. If I re-execute the code, I tell the interpreter to register another hook. The state in a Jupyter notebook is not tied to what's visible in the cells. If you want to redisplay the model, do the registering of the hook in a different cell then displaying the model or clear the hook registry beforehand. If you want different hooks for different models within the same kernel, then there is indeed a problem ... |
I would be rather surprised if I wasn't allowed to have two IPythonViz displays with their own hooks within the same notebook. Having multiple IPythonViz displays is pretty common in things like the SYDE556 lecture notes (and it's also very common to be re-executing the cells that define the model to modify things). So I think it would be strange if we didn't support this use case. Now, as you pointed out, we could add a function to clear the hook registry. Something like Note: the scenario I'm describing here is also a problem for my initial implementation #953 (you'd have to toss in some |
This is a proof-of-concept of implementing the hooks proposed in #953 with decorators to explicitely register the hooks. This would still need some work before it is ready to merge:
Hooks will be registered on a per page basis which should alleviate the main concern about this approach. In addition global hooks can be registered which allows to register hooks for IPythonViz (to be renamed to InlineGUI).