Resolve the NotifierNotFound error in testsuite by aaronayres35 · Pull Request #601 · enthought/chaco

aaronayres35 · 2021-04-02T18:53:26Z

closes #597

The current change is a little confusing but it gets the job done. So the original cause of the error is that axis is an enum trait which defaults to index. Thus, the first time _axis_changed is called, it is called with old="index", new="value". However, that means that we never set up the observer on the "index_mapper" trait, but we are now trying to remove it. Hence the NotifierNotFound error.

To resolve this, I intended to simply set up that observer initially. I tried doing this by overriding the __init__ method (while calling super), but this timing was incorrect. We hook the observer up to the self.plot object, but in the __init__ this is not the correct object as the plot changes. I then tried using a _plot_changed method, but this was also proving unsuccessful. It turns out the plot trait is really just a property that ends up mirroring component. Changing to _component_changed does as expected (hooks up listener to the correct object).

aaronayres35 · 2021-04-09T21:11:19Z

Okay so if I instead just replace _axis_changed with the

@observe('axis', post_init=True)
    def dumb(self, event):
        if event.old is not None:
            self.plot.observe(
                self.__mapper_changed, old + "_mapper", remove=True
            )
        if event.new is not None:
            self.plot.observe(self.__mapper_changed, new + "_mapper")

The warning goes away.

However this isn't really the right thing I don't think...
By default axis is "index". Ideally we want to have a listener set up such that when the plot object's index_mapper or value_mapper trait changes, we call the __mapper_changed method on this class. Hence the body of the current _axis_changed method. The problem is, if no one changes the axis trait, we never set that up. If someone does change it, for example by instantiating a RangeSelection object with axis="value" we end up trying to unhook that listener but it was never hooked up in the first place. By adding post init, when instantiating a RangeSelection object with axis="value" we simply do not call the change handler so we don't try to unhook/hookup any listeners. This "solves" the problem by preventing the NotifierNotFound as we no longer try to unhook a notifier that doesn't exist. But it doesn't truly solve the issue, as there is probably some reason we really do want these handlers to get set up...

Also this pattern of hooking/ unhooking listeners this way is still very strange to me because it assumes 1 of two things (at least I think):

what ever the initial value for the trait is will trigger the handler with old=None, new=the thing so the listener gets set up
or,
we only want to listen to a trait if it has been changed (but I can't come up with a rational explanation of why this would be the case in my brain). To be overly specific: For the RangeSelection instantiated with the default value for axis of "index", if the index_mapper trait on the plot changes we don't want __mapper_changed to be called. However, if we change axis to "value" and immediately back to "index", from then on if the index_mapper trait on the plot changes we do want __mapper_changed to be called.

The current fix works only because 1) above holds for component.
At this point, I am unsure of what a better alternative would be. As the code currently stands there is no Error, and I believe all the listeners we want to get hooked up / unhooked are. So for now I think I am going to simply flag this as ready for review, although I would like to have a discussion to clarify my understanding about this / what the best practice way to do this sort of thing should be going forward.
@rahulporuri I know this is an extremely overly wordy comment, but I hope my thinking made some sense and we can discuss this monday.

rahulporuri · 2021-04-13T04:18:24Z

... However, that means that we never set up the observer on the "index_mapper" trait, but we are now trying to remove it. Hence the NotifierNotFound error.

I should have paid more attention to this effort earlier. From what I can see, there is actually a bug here. If the listener isnt hooked up correctly at first, we should be able to reproduce this using a regression test. Once we have the regression test, I think we can also be confident about the change we are making.

…ailed)

aaronayres35 · 2021-04-13T17:54:16Z

+            )
+        if new is not None:
+            self.plot.observe(self.__mapper_changed, self.axis + "_mapper")
+


semi-relevant (?) ref: enthought/enable#739
See this class' base class, AbstractController's __init__method:

chaco/chaco/abstract_controller.py

Lines 16 to 18 in 3e83091

def __init__(self, component, *args, **kw):

self.component = component

super(AbstractController, self).__init__(*args, **kw)

Probably. Dynamic trait change handlers (@on_trait_change() and @observe()) need HasTraits.__init__() to run before the machinery is properly set up. Static trait change handlers like _component_changed don't need that machinery to be set up and will be run on the relevant attribute setting.

ah that is very good to know, thank you!

aaronayres35 · 2021-04-13T18:54:37Z

+    @unittest.mock.patch('chaco.tools.range_selection.RangeSelection.deselect')
+    def test_notifiers_connected(self, mocked_deselect):
+        plot_data = ArrayPlotData()
+        arr = np.arange(4.0)
+        plot_data.set_data("x", arr)
+        plot_data.set_data("y", arr)
+
+        plot = Plot(plot_data)
+
+        renderer = plot.plot(("x", "y"))[0]
+        renderer.bounds = [10, 20]
+        tool = RangeSelection(renderer)
+        renderer.tools.append(tool)
+
+        # attempt to trigger change handler for the index_mapper trait on the
+        # RangeSelection tool's plot
+        # assign a new mapper with same attrs
+        renderer.index_mapper = LinearMapper(
+            range=renderer.index_mapper.range,
+            stretch_data=renderer.index_mapper.stretch_data
+        )
+
+        mocked_deselect.assert_called_once()


This test fails, and the error is still observed if the fix in the file above is replaced with:

@observe("component") def _component_updated(self, event): if event.old is not None: self.plot.observe( self.__mapper_changed, self.axis + "_mapper", remove=True ) if event.new is not None: self.plot.observe(self.__mapper_changed, self.axis + "_mapper")

seems the timing at which notifiers are setup/called is different for the magic named methods vs decorating with observe?

…d methods

rahulporuri · 2021-04-14T12:39:57Z

-
-    def _axis_changed(self, old, new):
-        if old is not None:
-            self.plot.observe(


So plot is _plot if it exists or else it is component. If we're hooking things up directly to component, then we're going to have bugs if plot property i.e. _plot is ever set.

i guess we can use trait from observe and hook up _plot.index_mapper and _plot.value_mapper if _plot exists.

Actually, what happens if we listen to plot.index_mapper and plot.value_mapper?

Actually, what happens if we listen to plot.index_mapper and plot.value_mapper?

In this case the added test test_notifiers_connected fails with deselect never being called

I added _plot.index_mapper and _plot.value_mapper to the observe decorator, along with a test (which previously failed) for checking that if we do explicitly set plot, we still get notified of changes.

…ession" This reverts commit 306e8d9.

aaronayres35 · 2021-04-15T20:26:26Z

+        "component.index_mapper",
+        "component.value_mapper",
+        "_plot.index_mapper",
+        "_plot.value_mapper"


I actually don't think we want this optional=True here. _plot = Trait(None, Any).
Without the optional=True everything seems to work fine, and this might be confusing. whatever _plot gets set to should definitely have index_mapper and value_mapper traits.

It is possible I am missing something here and we need to specify them as optional for some reason, but the _plot trait is guaranteed to exist. If it is None it will not have index_mapper and value_mapper traits, but if it is ever set it should. And it appears that there are not problems encountered when it is None (ie observe isn't complaining raising an error or warning or anything)

I am happy to undo this revert if needed. @rahulporuri lmk what you think on this

can't we just use plot.index_mapper and plot.value_mapper?

I tried doing this but the added test fails
But I can look into it more to try to better understand why thats happening

aaronayres35 · 2021-04-16T14:01:37Z

Summary of my latest findings:
if we simple do @observe("plot.index_mapper,plot.value_mapper") instead, both the newly added tests (test_notifiers_connected and test_notifiers_connected_specify_plot) fail.

If I do that and change the definition of the plot property trait from plot = Property to plot = Property(observe="component,_plot") only the test_notifiers_connected test fails. The other passes.

If I do the above but even further define the plot trait as plot = Property(observe="component,component.index_mapper,_plot") the test_notifiers_connected test fails, but do see the change handler called (only event.name is plot so deselect is not called

From the traits docs:
"The property will fire a trait change notification if any of the traits specified by observe change."
So in this case since it is component.index_mapper that is changing, but component is unchanged, it looks like we are not able to get a change notification for plot.index_mapper. By adding component.index_mapper to observe in the property definition we get notified of a plot change but that isn'y really what we want.
I am unsure what the best approach is here... Perhaps this is actually a bug / motivation for new feature in traits?
I don't know what we would need to do to get a change to trigger with the change being the index_mapper on plot...
The current fix on this PR does what we want, but it is ugly and feels overly convoluted.

I am going to stop investigating this for now, but it is likely worth further discussion.

rahulporuri

Still LGTM

rahulporuri · 2021-04-23T04:33:24Z

I am unsure what the best approach is here... Perhaps this is actually a bug / motivation for new feature in traits?

It's probably worth asking the traits maintainer what the best solution for this problem is - is the solution we have the best available or are there better alternatives.

first stab at resolving the NotifierNotFound error

357263d

aaronayres35 mentioned this pull request Apr 2, 2021

Remove thing_changed Event traits if possible #604

Open

aaronayres35 changed the title ~~[WIP] Resolve the NotifierNotFound error in testsuite~~ Resolve the NotifierNotFound error in testsuite Apr 9, 2021

aaronayres35 requested a review from rahulporuri April 9, 2021 21:24

aaronayres35 added 4 commits April 13, 2021 12:04

Merge branch 'master' into fix-NotifierNotFound

6e95485

add a test to ensure handler is hooked up / called (this previously f…

1f174d9

…ailed)

add comment with issue number

9f64a74

flake8

c299e5a

aaronayres35 commented Apr 13, 2021

View reviewed changes

aaronayres35 added 2 commits April 13, 2021 15:20

decorarte the method we are observing directly, remove magically name…

4067035

…d methods

rename method to not look like a magic named method

51fcdce

rahulporuri reviewed Apr 14, 2021

View reviewed changes

aaronayres35 added 4 commits April 15, 2021 13:14

handle _plot changing case

0138600

be more explicit about traits being optional in observer expression

306e8d9

flake8

9dc4ba1

Revert "be more explicit about traits being optional in observer expr…

7b54518

…ession" This reverts commit 306e8d9.

aaronayres35 commented Apr 15, 2021

View reviewed changes

aaronayres35 requested a review from rahulporuri April 19, 2021 18:06

rahulporuri approved these changes Apr 23, 2021

View reviewed changes

aaronayres35 merged commit 1d168d1 into master Apr 23, 2021

aaronayres35 deleted the fix-NotifierNotFound branch April 23, 2021 13:50

coreysharris mentioned this pull request Nov 15, 2021

Bug in RangeSelection #822

Open

	def __init__(self, component, args, *kw):
	self.component = component
	super(AbstractController, self).__init__(args, *kw)

Conversation

aaronayres35 commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronayres35 commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahulporuri commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

rahulporuri commented Apr 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronayres35 commented Apr 2, 2021 •

edited

Loading

aaronayres35 commented Apr 9, 2021 •

edited

Loading

aaronayres35 commented Apr 16, 2021 •

edited

Loading