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

Support setting a link on traits #641

Open
InfiniteChai opened this issue Jan 30, 2021 · 4 comments · May be fixed by #642
Open

Support setting a link on traits #641

InfiniteChai opened this issue Jan 30, 2021 · 4 comments · May be fixed by #642

Comments

@InfiniteChai
Copy link

I'm looking to start using a standard pattern in our UIs to split our model and views, similar to the points discussed in jupyter-widgets/ipywidgets#2296. However when extending to more complex or nested views it's rather inconvenient to have to define all the links separately as we need to hold a reference to every field.

Instead I'd been keen to introduce the ability to set links directly on traits. This way I can bind to the view's traits without needing to hold a reference to the object. Proposing something along the following lines so that it's backwards compatible with current traitlet usage.

from traitlets import SourceLink, Float, HasTraits
import ipywidgets as widgets

class Model(HasTraits):
   a = Float()

def view(model):
    return widgets.HBox([
        widgets.Label("Value A"),
        widgets.FloatText(value = SourceLink(model, "a"))
    ])

x1 = Model()
view(x1)

Having tested locally I think this is easily achievable with minimal changes. However before submitting a PR I wanted to check if any views or comments on the idea or how to approach it.

@InfiniteChai InfiniteChai linked a pull request Jan 30, 2021 that will close this issue
@InfiniteChai
Copy link
Author

Thought made sense to post the PR as well, to see the proposed change to support this.

@rmorshea
Copy link
Contributor

rmorshea commented Jan 31, 2021

when extending to more complex or nested views it's rather inconvenient

Can you show an example that demonstrates the inconvenience? I think it would help in determining whether or not this is a necessary addition. At the moment we're trying to be conservative about adding new features to traitlets since there aren't that many people who maintain it.

@InfiniteChai
Copy link
Author

Hi @rmorshea. So unfortunately I can't reference you to an actual example we have as it's proprietary. However to explain the situation we've now got models (economic instruments) that 50 or more different properties that we want to display.

This means that we have over 100 lines of replicating, redundant binding code (to define the variables then bind them as demonstrated below) on every view which is starting to make the code rather unreadable and losing focus from the key point we want developers to focus on, which is the view itself.

def view(model):
    a_view = widgets.FloatText()
    b_view = widgets.FloatText()
    ...
    z_view = widgets.FloatText()

    v = widgets.HBox([
        widgets.Label("Value A"),
        a_view,
        ...
        widgets.Label("Value Z"),
        z_view, 
    ])

    link((a_view, "value"), (model, "a"))
    link((b_view, "value"), (model, "b"))
    ...
    link((z_view, "value"), (model, "z"))
    return v

As I said, this is an inconvenience and the change does not introduce functionality which cannot already be achieved in a separate way (as demonstrated) but it does make for more concise and readable code when building this model/view paradigm.

@rmorshea
Copy link
Contributor

rmorshea commented Feb 1, 2021

Could this be solved with a utility function that can create more than one link at a time?

def link_many(source: Any, target: Any, attribute_mapping: Dict[str, str]): -> None: ...

Example usage:

link_many(
    source,
    target,
    {
        "source_attr_a": "target_attr_a",
        "source_attr_b", "target_attr_b",
    }
)

I bring this up because, while I do like the aesthetics of the SourceLink idea, I'd rather not impact TraitType.__set__ to do it if possible. traitlets already has some performance issues, so it'd be best to avoid incurring the cost of an extra isinstance(obj, SourceLink) check on every attribute assignment for a use case that is somewhat niche.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants