enthought / chaco

Chaco is a Python package for building interactive and custom 2-D plots.
http://docs.enthought.com/chaco/
Other
292 stars 99 forks source link

Removing a data source makes `DataView` objects inconsistent #821

Closed siddhantwahal closed 1 year ago

siddhantwahal commented 2 years ago

Problem Description Removing a data source from a DataView instance leaves it internally inconsistent.

Reproduction Steps: This snippet demonstrates the inconsistency:

import numpy as np

from chaco.api import DataView
from chaco.grid_data_source import GridDataSource

source = GridDataSource(xdata=np.array([1, 2, 4]), ydata=np.array([7, 8, 9]))
component = DataView()
print(f"Initial output: {component.map_screen(np.array([0, 1]))}")
component.range2d.add(source)  # (1)
component.map_screen(np.array([0, 1]))  # (2)
component.range2d.remove(source)  # (3)
print(f"Final output: {component.map_screen(np.array([0, 1]))}")

Output:

Initial output: [[0. 0.]]
Final output: [inf inf]

Initially, without any data sources, the output of component.map_screen is [[0, 0]]. When a data source is added and then removed, the bounds on component.value_mapper.range to refreshed to (-inf, inf). However, component.value_mapper._cache_valid isn't reverted from True to False. (It's set to True as a consequence of (2) in the snippet above.) As a result, subsequent calls to component.map_screen produce infs. This infs in turn have the potential to turn into nans, raising exceptions in unexpected places downstream.

Expected behavior: I think the output should be [[0., 0.]] again, (or more precisely, component.value_mapper._cache_valid should revert to False if component.value_mapper.range.refresh() is called. However, I'm not sure and opening this issue for discussion.

OS, Python version: macOS Catalina 10.15.7, Python 3.6.13

corranwebster commented 1 year ago

So the root cause appears to be that in the branch where there is no data and so the low and high values get set to ±inf, the update trait does not get fired, and so the mapper doesn't get the _cache_valid trait reset. If we fire that event it will then return [[0 0]] as suggested.

From a UX perspective, I'm not 100% sure that this is the right thing, however. It may be more natural to have the range stay fixed at its previous values when the last data is removed from the Range to prevent sudden resets of the view. But on the other hand, this may be OK when in "auto" or "track" modes.

I'll think about this some more.