Skip to content

Catch nans before they cause crash in auto_ticks #636

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 10 commits into from
Apr 13, 2021
Merged

Catch nans before they cause crash in auto_ticks #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65ab0cc
catch nans before they cause crash
aaronayres35 Apr 8, 2021
3a0314e
if there no data or screen range, we shouldn't even try to pan
aaronayres35 Apr 9, 2021
299099e
undo previous change and dont return an array from map_data
aaronayres35 Apr 9, 2021
a6b755a
undo changes since the original fix
aaronayres35 Apr 9, 2021
bb271de
add a comment for the fix
aaronayres35 Apr 9, 2021
e7de578
fix typo and add regression test
aaronayres35 Apr 9, 2021
80aa064
skip test on null backend
aaronayres35 Apr 9, 2021
d75d2de
Apply suggestions from code review
aaronayres35 Apr 13, 2021
23cc404
Merge branch 'master' into dont-crash-empty-plot
aaronayres35 Apr 13, 2021
89a1a5e
flake8
aaronayres35 Apr 13, 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
42 changes: 42 additions & 0 deletions chaco/tests/test_plot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,16 @@

from numpy import alltrue, arange, array

from enable.api import ComponentEditor
from enable.testing import EnableTestAssistant
from traits.api import HasTraits, Instance
from traits.etsconfig.api import ETSConfig
from traitsui.api import Item, View

# Chaco imports
from chaco.api import ArrayPlotData, Plot, DataRange1D, PlotGraphicsContext
from chaco.default_colormaps import viridis
from chaco.tools.api import PanTool, ZoomTool


class PlotTestCase(unittest.TestCase):
Expand Down Expand Up @@ -91,3 +98,38 @@ def test_text_plot(self):
gc.render_component(plot)
actual = gc.bmp_array[:, :, :]
self.assertFalse(alltrue(actual == 255))


class EmptyLinePlot(HasTraits):
plot = Instance(Plot)
x = []
y = []
traits_view = View(
Item('plot', editor=ComponentEditor(), show_label=False),
width=500,
height=500,
resizable=True
)

def _plot_default(self):
plot = Plot(ArrayPlotData(x=self.x, y=self.y))
plot.plot(("x", "y"), type="line", color="blue")
plot.tools.append(PanTool(plot))
plot.overlays.append(ZoomTool(plot, zoom_factor=1.1))
return plot


# regression test for enthought/chaco#529
@unittest.skipIf(ETSConfig.toolkit == "null", "Skip on 'null' toolkit")
class TestEmptyPlot(unittest.TestCase, EnableTestAssistant):

def test_dont_crash_on_click(self):
from traitsui.testing.api import UITester
tester = UITester()
empty_plot = EmptyLinePlot()

with tester.create_ui(empty_plot):
self.press_move_release(
empty_plot.plot,
[(1, 1), (25, 25), (50, 50), (100, 100)],
)
5 changes: 5 additions & 0 deletions chaco/ticks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
finfo,
float64,
floor,
isnan,
linspace,
log10,
minimum,
Expand Down Expand Up @@ -242,6 +243,10 @@ def auto_ticks(
An array of tick mark locations. The first and last tick entries are the
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri Apr 13, 2021

Choose a reason for hiding this comment

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

nit - this is related but also tangential. the docstring says that this method returns an array - i am not sure if that means that we should strictly return an empty array.

this goes back to the general issue you mentioned of checking and enforcing return types across chaco. i think this can be an umbrella issue where we accumulate and fix issues that we discover.

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 13, 2021

Choose a reason for hiding this comment

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

I saw this too, but then at the end of the method it appears to return just a list so an empty list is consistent.
Perhaps I should update the docstring instead? Or maybe they both really should be arrays?

Copy link
Copy Markdown
Contributor Author

@aaronayres35 aaronayres35 Apr 13, 2021

Choose a reason for hiding this comment

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

I think I am going to push this through for now, but this can be re-evaluated as part of #662

axis end points.
"""
# if a NaN makes it here, we cant determine tick mark locations. Return
# empty list to prevent crash. Motivated by enthought/chaco#529
if isnan([data_low, data_high, bound_low, bound_high]).any():
Comment thread
rahulporuri marked this conversation as resolved.
return []

is_auto_low = bound_low == "auto"
is_auto_high = bound_high == "auto"
Expand Down