Skip to content
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

Add a get_interact_value() method to customize the value passed to interactive functions #725

Merged
merged 1 commit into from
Sep 16, 2016

Conversation

jdemeyer
Copy link
Contributor

Let W be a widget. If that is used for an interactive (@interact) function, the function receives W.value. I propose to allow to customize this by adding a method W.get_value() to return the value pased to the interactive function. By default, this is just W.value but custom widgets could change this.

This solves the same problem as #724 but perhaps with a cleaner API.

Context: this is to support analogous functionality from SageNB to help with OpenDreamKit/OpenDreamKit#94.

@SylvainCorlay
Copy link
Member

Although this only works with widgets holding a value attribute - which are also the ones supported by interact...

@jdfreder
Copy link
Contributor

Can you provide more details? I understand you eval proposal, but I'm having trouble understanding this alternative one. Is the intention that accompanying this suggestion, there'd be a "Delegate" widget or something, which on set .value a function would be triggered, of which the output would be get_value?

@jdemeyer
Copy link
Contributor Author

I'm having trouble understanding this alternative one

This pull request obviously does not change anything for existing code, it just adds a hook.

My idea was to allow something like

class EvalText(ipywidgets.Text):
    def get_value(self):
        return eval(self.value)

This is something that SageNB supports. It allows passing essentially arbitrary Python objects as arguments for interactive functions.

But obviously, this pull request allows anything in get_value(), even unrelated to self.value. That's why I think it's a good idea, it's a very general hook.

@SylvainCorlay
Copy link
Member

I am not really sure about this in general. My view is that is you want to do something non-trivial with widgets, you declare and wire them explicitly.

Interact' s goal to be completely auto-magic: based on introspection of default values, it creates a simple GUI automatically to interact with a function. Adding hooks exposes a complex inner machinery that is very likely to go south eventually.

@jdemeyer
Copy link
Contributor Author

Adding hooks exposes a complex inner machinery that is very likely to go south eventually.

Nothing forces anybody to use those hooks. It's completely fine for me if ipywidgets itself never uses that hook.

@SylvainCorlay
Copy link
Member

Adding hooks exposes a complex inner machinery that is very likely to go south eventually.

Nothing forces anybody to use those hooks. It's completely fine for me if ipywidgets itself never uses that hook.

Although it increases the API surface. If we change that something that breaks that one thing, it becomes an issue.

@jasongrout
Copy link
Member

jasongrout commented Aug 22, 2016

Having a transformation function that intercepts values from the widgets before they are passed into the interact function is a nice generalization, I think. It allows a user to construct some more sophisticated controls easily (for example, if we had a grid control, we could easily make a numpy array control by taking the value of the grid and converting it to a numpy array).

On the other hand, a transformation control like Jon hints at is more composable, and doesn't just apply to controls with a value field. I think having a transformation control would look like:

# in ipywidgets
class Transform(...):
    def __init__(self, control, attribute, ...):
        # observe the control's attribute
        # on change, apply self.transform and set self.value

    def transform(self, value):
        return value

# in user code
class Eval(ipywidgets.transform):
    def transform(self, value):
        return eval(value)

@interact(x = Eval(ipywidgets.Text, 'value'))
def _(x):
    print(x)

Another nice thing about this approach is that it lets us use attributes other than value in interact controls, since the identity transform essentially maps a given attribute to the value attribute.

@jasongrout
Copy link
Member

jasongrout commented Aug 22, 2016

This transform control is essentially a python dlink control with the endpoint being itself and the value attribute. With the added advantage that it takes a single line to declare and wire, instead of three lines to set up two controls and wire them together.

@jdemeyer
Copy link
Contributor Author

Another nice thing about this approach is that it lets us use attributes other than value in interact controls

That is possible with my branch too, nobody says that widget.get_value() needs to access widget.value.

Anyway, I don't care much how my use case is solved, I just want a solution. My solution is very simple, works right now and doesn't break anything in ipywidgets.

@jdemeyer
Copy link
Contributor Author

Another use-case for this branch or "transformations" in general is to change the type of slider values. I could use this branch to make a IntSlider which passes a different type of integer to the interactive function. See also #721.

@minrk
Copy link
Contributor

minrk commented Aug 30, 2016

@jdemeyer perhaps the existing to_json/from_json transforms are enough for what you need:

from ipywidgets import interact, Text
from traitlets import Any

def to_str(x, obj):
    return str(x)

def from_str(x, obj):
    return eval(x)

class Eval(Text):
    value = Any().tag(sync=True, to_json=to_str, from_json=from_str)

@interact(x=Eval('1'))
def foo(x):
    print(repr(x), type(x))

or for a custom IntSlider:

import numpy as np
from ipywidgets import interact, IntSlider
from traitlets import Instance, validate

def to_int(x, obj):
    return int(x)

def from_int(x, obj):
    return np.int64(x)

class MyIntSlider(IntSlider):
    value = Instance(np.int64).tag(sync=True, to_json=to_int, from_json=from_int)

@interact(x=MyIntSlider(np.int64(5)))
def foo(x):
    print(repr(x), type(x))

The difference I see between existing from_json and get_value proposed here is that the from_json transform happens between class.value and the frontend, rather than between class.value and the @interact-decorated function, which seems preferable to me.

@jdemeyer
Copy link
Contributor Author

@jdemeyer perhaps the existing to_json/from_json transforms are enough for what you need:

I need to check, but it could be.

@minrk
Copy link
Contributor

minrk commented Aug 30, 2016

Great! Let me know what you find, and hopefully we can get something working for you.

@jdemeyer
Copy link
Contributor Author

Do you consider the JSON interface for the widgets part of your public API?

@minrk
Copy link
Contributor

minrk commented Aug 30, 2016

yes, definitely.

@minrk
Copy link
Contributor

minrk commented Aug 30, 2016

It's not well documented (at all?), and certainly an advanced usage, but it is a public API and needed for many custom widgets developed outside ipywidgets.

@SylvainCorlay
Copy link
Member

If you assign a new to_json to a trait metadata it is going to change how
that thing is serialized for the front end and you will break the widget.

I am not sure that I really understand the purpose of get_value though.

On Aug 30, 2016 1:27 PM, "Min RK" notifications@github.com wrote:

It's not well documented (at all?), and certainly an advanced usage, but it
is a public API and needed for many custom widgets developed outside
ipywidgets.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#725 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACSXFg0jCEGDbY67UWWP146oNxKNqugMks5qlBOWgaJpZM4JnhWZ
.

@minrk
Copy link
Contributor

minrk commented Aug 30, 2016

If you assign a new to_json to a trait metadata it is going to change how
that thing is serialized for the front end and you will break the widget.

it has to serialize to the same type for the widget to work, but it works just fine for existing widgets, including IntSlider and Text above. The goal is to allow changing the type of .value, mapping to an existing widget. This works for alternate integer types, for instance, as well as eval of strings.

@SylvainCorlay
Copy link
Member

In term of use case, in addition to the function passed to interact, you want to have a different API to modify the input parameters by assigning a function attribute to the widget. It seems very cumbersome to me. I don't quite get what this transformation would do that could not be part of the main function passed to interact.

@SylvainCorlay
Copy link
Member

One thing that I think would be worth considering is to formalize the value widget pattern, that is a widget that holds a value trait, and used as an input field for that value. It is the underlying assumption of interact but not the case for every widget (bqplot scales, layout widgets, etc).

So having a base ValueWidget base class or something like this would make sense. These widgets could have extra APIs that are not necessarily in the base DOMWidget.

@jdemeyer
Copy link
Contributor Author

I don't quite get what this transformation would do that could not be part of the main function passed to interact.

Of course it can be part of the interactive function. But this is something that I do not want because it is cumbersome. For example, I may want to re-use existing functions which take certain arguments. And I want some common "standard" transformations, for example the eval() of strings which is widely used in SageNB interacts.

@jdemeyer
Copy link
Contributor Author

class Eval(Text):
    value = Any().tag(sync=True, to_json=to_str, from_json=from_str)

I admit that this traitlets stuff is just magic to me.

Is there a way to make the above work with to_json and from_json methods instead of global functions? The actual use case is slightly more complicated and I need access to self.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Aug 31, 2016

to_json and from_json take two arguments, the second one being the HasTraits instance. They are not methods of HasTraits or of TraitType.

@jdemeyer
Copy link
Contributor Author

to_json and from_json take two arguments, the second one being the HasTraits instance.

I see. So basically the second argument is self.

@SylvainCorlay
Copy link
Member

I see. So basically the second argument is self.

Hum I don't think so. At some point we made to_json a method of the Trait Type but never the HasTraits instance...

We often have things like

class Foo(HasTraits):
    bar = Array().tag(to_json=array_to_json)

class Bar(HasTraits):
    baz = Array().tag(to_json=array_to_json)
    tar = Instance(Widget).tag(to_json=widget_to_json)

array_to_json is not a method of Foo or Bar, and it does not make sense to have every possible serializer as a method of HasTraits. It is a pluggable thing.

@jdemeyer
Copy link
Contributor Author

At some point we made to_json a method of the Trait Type but never the HasTraits instance...

I meant it's not actually a method but it can be used as if it was a method where self is the last argument. The point is that I can write

def from_str(x, self):
    return self.eval_function(x)

@jdemeyer
Copy link
Contributor Author

Just adding that self is only a name, but it helps me to think as if it was a method.

@SylvainCorlay
Copy link
Member

I prefer the approach of Jon and Jason with the delegate widget over dynamically changing to_json and from_json or assigning new methods to the base widget.

@jdemeyer
Copy link
Contributor Author

I prefer the approach of Jon and Jason with the delegate widget over dynamically changing to_json and from_json or assigning new methods to the base widget.

I personally don't care as long as it works. The to_json/from_str thing works right now, so that is what I will use for now.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Aug 31, 2016

I prefer the approach of Jon and Jason with the delegate widget over dynamically changing to_json and from_json or assigning new methods to the base widget.

I personally don't care as long as it works. The to_json/from_str thing works right now, so that is what I will use for now.

But it does not make sense! If you modify to_json you are going to change the value that is transmitted to the javascript, which may not even make sense for the front end.

@jdemeyer
Copy link
Contributor Author

Well, one thing which is bad with Min's proposal: whenever the Javascript value of a Text control is updated, from_json() is called to compute the Python value but then also to_json() is called to update the Javascript value from the Python value. The latter step should not be done because nothing guarantees that the round-trip JSON -> Python -> JSON is lossless.

@minrk
Copy link
Contributor

minrk commented Aug 31, 2016

Good point. I wouldn't object in general to a .get_interact_value() (I don't think generic .get_value() is appropriate, because this is really for interact, not in general, where .value is already the accessor for the actual value) method that can be a transform from the widget's value to what is passed to interact. While the generic transform(Text('x')) makes sense in principle, this seems a lot simpler and easier to use.

@SylvainCorlay
Copy link
Member

@minrk the get_interact_value could be there in the proposed ValueWidget base class for all widgets representing a main value.

@SylvainCorlay
Copy link
Member

@minrk the get_interact_value could be there in the proposed ValueWidget base class for all widgets representing a main value.

What do you think of this ValueWidget base class for widgets that hold a value attribute and are suitable for use in interact?

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Sep 4, 2016

What do you think of this ValueWidget base class for widgets that hold a value attribute...

I have nothing against this but I also don't really see the point of introducing this extra class.

...and are suitable for use in interact?

Part of the point of this pull request is that "widget holding a value attribute" and "widget suitable for use in interact" are not necessarily the same thing. So -1 to this part.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Sep 4, 2016

What do you think of this ValueWidget base class for widgets that hold a value attribute...

I have nothing against this but I also don't really see the point of introducing this extra class.

The reason is that not all widgets can be used for interact. For example interact(x=HBox([])) or interact(x=figure) where fig is an instance of bqplot.Figure don't make sense.

...and are suitable for use in interact?

Part of the point of this pull request is that "widget holding a value attribute" and "widget suitable for use in interact" are not necessarily the same thing. So -1 to this part.

Why is it not the same thing?

@jdemeyer jdemeyer changed the title Add a get_value() method to customize the value passed to interactive functions Add a get_interact_value() method to customize the value passed to interactive functions Sep 8, 2016
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Sep 8, 2016

This new version adds a ValueWidget class as requested by @SylvainCorlay

@minrk minrk added this to the 6.0 milestone Sep 9, 2016
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Sep 9, 2016

I just thought about yet another use case for get_interact_value(): imagine a Box containing multiple widgets which corresponds to one argument of an interactive function. Say, an argument which takes a N-tuple, represented by a Box with N children. Or a poor man's complex number widget represented by a slider for the real part and a slider for the imaginary part.

Then this get_interact_value method could be used to build the value from the values of the children.

@SylvainCorlay
Copy link
Member

👍 this will need a rebase!

@jdemeyer
Copy link
Contributor Author

👍 this will need a rebase!

Sorry for that. It's done now.

@SylvainCorlay SylvainCorlay merged commit 3adddcf into jupyter-widgets:master Sep 16, 2016
@SylvainCorlay
Copy link
Member

✨ 🎉

@jdemeyer
Copy link
Contributor Author

Cool, thanks!

jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Oct 11, 2016
minrk added a commit that referenced this pull request Oct 13, 2016
PR #725 broke the fixed() functionality of @interact. This fixes it.
@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants