Skip to content

Link tranform#393

Merged
minrk merged 6 commits intoipython:masterfrom
francisco-dlp:link_tranform
Aug 15, 2017
Merged

Link tranform#393
minrk merged 6 commits intoipython:masterfrom
francisco-dlp:link_tranform

Conversation

@francisco-dlp
Copy link
Contributor

Add transform argument to link that takes a tuple with the forward and backward transform.

@minrk
Copy link
Member

minrk commented May 3, 2017

cc @SylvainCorlay for review

@SylvainCorlay
Copy link
Member

Thanks.

To provide some background, one reason why we only added it to the directional case originally is that one needs to ensure that the two tranforms are inverses of each other to avoid infinite bouncing behavior.

@francisco-dlp
Copy link
Contributor Author

francisco-dlp commented May 8, 2017

Thanks for the feedback. Why would two transforms that are not inverses of each other cause bouncing behaviour?

I think that this is an useful feature. For example, say that I define a span selector and I want to fix its width without changing the class definition. With transform it is as simple as:

import traitlets

class SpanSelector(traitlets.HasTraits):
    x1 = traitlets.Float()
    x2 = traitlets.Float()

ss = SpanSelector()
fix_width = traitlets.link((ss, "x1"), (ss, "x2"), (lambda x: x + 10, lambda x: x - 10))

Of course things may not work as expected if the transforms are not inverses, but I think that it is fair to assume that the users know what they're doing.

I have merged the current master into the branch to solve the merge conflicts.

@maartenbreddels
Copy link
Contributor

I think this is a useful feature, and it would be nice to have this in ipywidgets as well. Can we check in _update_source and _update_target if the transformations are actually the inverse? And raise an Exception is they differ.

@francisco-dlp
Copy link
Contributor Author

I think that checking if the transformations are the inverse of each other is not trivial e.g. they may be the inverse of each other only in a certain domain.

@rmorshea
Copy link
Contributor

rmorshea commented May 9, 2017

@francisco-dlp because we check link.updating I don't think we have to worry about bouncing -
the link will always suppress change notifications while it's "busy". However, this also means that it would be possible to break the consistency of the transform relationship:

class MyClass(HasTraits):
    i = Int()
    j = Int()
    
    @observe("j")
    def another_update(self, change):
        self.i = change.new * 2

mc = MyClass()

trans = (lambda x: x**2, lambda x: x**(0.5))
l = link((mc, "i"), (mc, "j"), trans)

mc.i = 2
>>> mc.i
8
>>> mc.j
4

Inconsistent states will result when a consequence of a link's update changes either the source or the target trait. To resolve this issue we could raise if, after the update context is closed, the transform's relationship has been broken.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented May 9, 2017

I think that checking if the transformations are the inverse of each other is not trivial e.g. they may be the inverse of each other only in a certain domain.

That is why we did not implement bidirectional transform originally!

If this was to be added, it would be the responsibility of the user to provide inverses of the transforms

@francisco-dlp
Copy link
Contributor Author

@rmorshea, I think that the issue that you mention is unrelated to this pull request e.g.:

class MyClass(HasTraits):
    i = Int()
    j = Int()
    
    @observe("j")
    def another_update(self, change):
        self.i = change.new * 2

mc = MyClass()
l = link((mc, "i"), (mc, "j"))

mc.i = 2
>>> mc.i
4
>>> mc.j
2

@SylvainCorlay, there is of course a danger of things not working as expected if the arguments passed to a function are wrong or inconsistent, but this is true in general. Hence, I don't see why this should be an issue for adding an useful feature in this case.

@maartenbreddels
Copy link
Contributor

If it doesn't cause bouncing around, and we just say it's the responsibly of the user/programmer to make sure the transformations are fine I don't see a reason not to implement it.

@rmorshea
Copy link
Contributor

rmorshea commented May 9, 2017

@francisco-dlp, true - inconsistency was possible before - however if we decided that we should raise when it's encountered, that would impact how we implement transforms.

@francisco-dlp
Copy link
Contributor Author

@rmorshea, I have modified the code to raise a TraitError when the link doesn't work as intended. Thanks for pointing this out, as I think that it'll help prevent some hard to debug issues.

@francisco-dlp
Copy link
Contributor Author

If anybody would like to use this feature while waiting for this PR to get merged (or not) you could try link_traits. The purpose of the package is to implement the dlink and link functions of traitlets for enthought traits. It can be used to link traits to traits, traits to traitlets and (redundantly) traitlets to traitlets.

@minrk minrk added this to the 5.0 milestone Aug 15, 2017
@minrk minrk merged commit 6fc7797 into ipython:master Aug 15, 2017
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-major Major change in 5.0 need proper documentation labels Jun 4, 2020
@Carreau Carreau removed the 5.0-re-review Need to re-review for potential API impact changes. label Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.0-major Major change in 5.0 need proper documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants