-
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
Provide function for HTML display #755
base: master
Are you sure you want to change the base?
Conversation
By analyzing the blame information on this pull request, we identified @tcstewar, @Seanny123 and @bjkomer to be potential reviewers |
TODO: - remove dead code - merge nengo/nengo-gui#755
Thoughts on this? |
Interesting... I like the direction this is going in. It looks to me like it ends up being much more explicit, and avoids a bit of the magic that the current method relies on (and has tripped up @studywolf and myself a few times). Just to check, is this the sort of way you're thinking of using it? def timer_function(t):
if t < 1.0:
return 0
elif t < 2.0:
return 0
else:
return 1
def timer_html(t, x):
if t < 1.0:
return '<h1>Ready...</h1>'
elif t < 2.0:
return '<h1>Set...</h1>'
else:
return '<h1>Go!</h1>'
timer_function._nengo_html_function_ = timer_html
timer = nengo.Node(timer_function) I do think we'd want to implement it in such a way that the old method is deprecated, rather than suddenly unsupported, though. Hmm... one way to do that might just be to stick with |
You can see how I plan to use it in nengo/nengo-extras#22. Checking for callable |
One other weirdness comes to mind: right now, the class NodeFunc(object):
def __call__(self, t, x):
a, b = self.do_something_complex()
self.b = b
return a
def _nengo_html_(self, t, x):
return self.b So I think I'd recommend changing the self.data.append((t, self.html_function(t, x))) rather than calling it in the |
That's a good point. I was assuming it would not depend on any state information, but there's no reason we need to have that restriction. |
Also allows Processes to define custom HTML for viewing.
Made the changes @tcstewar suggested. Ready for re-review. |
TODO: - remove dead code - merge nengo/nengo-gui#755 - Update other examples currently using image_display_function - Allow for different scaling factors (like with image_display_function)
self.obj = obj | ||
self.obj_output = self.obj.output | ||
self.callable_html = callable(obj.output._nengo_html_) | ||
if self.callable_html: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this attribute is checked in almost every method makes me wonder if this might be a case for polymorphism. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Maybe provide a helper function that automatically instantiates the right class. Something like
def HTMLView(obj):
if callable(obj.output._nengo_htlm):
return CallableHTMLView(obj)
else
return StaticHTMLView(obj)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this out. In some ways it's cleaner, but the inheritance does add it's own form of cognitive complexity. Frankly, I'm just not sure if this class is large enough to really benefit from inheritance.
And now, I'm getting an error: ConfigError: Type 'CallableHTMLView' is not set up for configuration. Call configures('CallableHTMLView') first.
I've put the code in the html-function-refactor
branch. In the spirit of reviewers making the changes, you guys are welcome to run with that and try to figure out the error. But I don't have time to sink into that, and I think that what I have here is fine (or at least not worth the effort).
self.data = collections.deque() | ||
|
||
self.obj = obj | ||
self.obj_output = self.obj.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference between self.obj_output
and self.obj.output
? There's one line self.obj.output = self.obj_output
in a function which is doing the opposite assignment and I don't understand why. The two variables don't seem to be kept in sync and it seems to be random which one is used where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So self.obj_output
always represents the original node function. If not callable_html
, then we need to keep this around since we overwrite the node output, and need to be able to set it back in remove_nengo_objects
. It's not random which is used where: self.obj.output
is only used when we're setting or re-setting the node output, and self.obj_output
is used whenever we want to call the old node function. This is consistent with how this class currently works in master
.
In case you were wondering: Why didn't I just do it the way that @tcstewar suggested? Why is my code more complicated? The reason I created callable |
I think this if fine now. As @hunse graciously pointed out, the refactoring is non-trivial, unhelpful and we're going to refactor when we convert this to TypeScript anyways. |
The old way of doing custom HTML display required a node function to put HTML in a
_nengo_html_
attribute on the Node output. Also, the node output had to be callable, which is not the case for Processes.This allows the user to provide a function for custom HTML generation, which is a more flexible solution. Processes can now define functions to display themselves with custom HTML. Also, if you wanted changing HTML in the old setup, you had to rewrite
_nengo_html_
on the node function, which could be problematic if the same function was passed to more than one node. This gets around that problem.I had assumed that static HTML would be uncommon so I didn't accommodate it, but it it is something we want to do regularly, we should maybe special-case it. With what I have right now, you would need to make a function that just returns the same HTML each time, which is a bit more work and a bit more overhead if you just want static.
This is WIP since I have not yet fixed the examples to work with this, but that should be easy once we decide on how it's going to work. So let me know what you think.
When this gets merged, I can add a Process for presenting images to nengo_extras that will make it easy for people to view images they're presenting in the GUI.