Skip to content

Resolve the NotifierNotFound error in testsuite #601

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.

Sign up for GitHub

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

Merged
aaronayres35 merged 11 commits into from
Apr 23, 2021
Merged

Resolve the NotifierNotFound error in testsuite #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
357263d
first stab at resolving the NotifierNotFound error
aaronayres35 Apr 2, 2021
6e95485
Merge branch 'master' into fix-NotifierNotFound
aaronayres35 Apr 13, 2021
1f174d9
add a test to ensure handler is hooked up / called (this previously f…
aaronayres35 Apr 13, 2021
9f64a74
add comment with issue number
aaronayres35 Apr 13, 2021
c299e5a
flake8
aaronayres35 Apr 13, 2021
4067035
decorarte the method we are observing directly, remove magically name…
aaronayres35 Apr 13, 2021
51fcdce
rename method to not look like a magic named method
aaronayres35 Apr 13, 2021
0138600
handle _plot changing case
aaronayres35 Apr 15, 2021
306e8d9
be more explicit about traits being optional in observer expression
aaronayres35 Apr 15, 2021
9dc4ba1
flake8
aaronayres35 Apr 15, 2021
7b54518
Revert "be more explicit about traits being optional in observer expr…
aaronayres35 Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions chaco/tools/range_selection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
Instance,
Int,
List,
observe,
Property,
Str,
Trait,
Expand Down Expand Up @@ -676,13 +677,12 @@ def _determine_axis(self):
else:
return 0

def __mapper_changed(self, event):
self.deselect()

def _axis_changed(self, old, new):
if old is not None:
self.plot.observe(
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.__mapper_changed, old + "_mapper", remove=True
)
if new is not None:
self.plot.observe(self.__mapper_changed, new + "_mapper")
@observe([
"component.index_mapper",
"component.value_mapper",
"_plot.index_mapper",
"_plot.value_mapper"
Comment on lines +681 to +684
Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

])
def _axis_mapper_updated(self, event):
if event.name == self.axis + "_mapper":
self.deselect()
52 changes: 52 additions & 0 deletions chaco/tools/tests/test_range_selection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import numpy as np

from chaco.api import LinearMapper
from chaco.array_plot_data import ArrayPlotData
from chaco.plot import Plot
from chaco.tools.range_selection import RangeSelection
Expand Down Expand Up @@ -75,3 +76,54 @@ def test_selection_no_warning(self):
tool.selection = (1.5, 3.5)
tool.selection = [1.0, 2.0]
tool.selection = None

# regression test for enthought/chaco#597
@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]
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()

# regression test for enthought/chaco#597
@unittest.mock.patch('chaco.tools.range_selection.RangeSelection.deselect')
def test_notifiers_connected_specify_plot(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]
tool = RangeSelection(renderer)
renderer.tools.append(tool)

new_renderer = plot.plot(("x", "y"), type='scatter')[0]
tool.plot = new_renderer

# attempt to trigger change handler for the index_mapper trait on the
# RangeSelection tool's plot
# assign a new mapper with same attrs
new_renderer.index_mapper = LinearMapper(
range=new_renderer.index_mapper.range,
stretch_data=new_renderer.index_mapper.stretch_data
)

mocked_deselect.assert_called_once()