AbstractPlotRenderers return different array shapes when passing a single point to map_screen

[mdsmarte]

Base2DPlot and BaseXYPlot both implement the AbstractPlotRenderer interface, in particular the map_screen method.

When a single (x, y) point is passed to Base2DPlot.map_screen, the returned array is of the form [[x, y]] (nested, shape = (1, 2)), as set by GridMapper.map_screen:

https://github.com/enthought/chaco/blob/4a6683521cce012b5df4830d64f41518798fc817/chaco/grid_mapper.py#L114-L123

When a single (x, y) point is passed to BaseXYPlot.map_screen, the returned array is of the form [x, y] (flattened, shape = (2,)):

https://github.com/enthought/chaco/blob/4a6683521cce012b5df4830d64f41518798fc817/chaco/base_xy_plot.py#L336-L353

This difference forces objects that interact with AbstractPlotRenderers to make an assumption about the behavior of the map_screen method. For example, the DataLabelTool assumes the map_screen return shape matches that of BaseXYPlot for a single point ((2,)):

https://github.com/enthought/chaco/blob/4a6683521cce012b5df4830d64f41518798fc817/chaco/tools/data_label_tool.py#L53-L54

And therefore this tool throws an exception when used with a Base2DPlot:

File "<REDACTED>/python3.6/site-packages/chaco/tools/data_label_tool.py", line 53, in drag_start
    pointx, pointy = label.component.map_screen(label.data_point)
ValueError: not enough values to unpack (expected 2, got 1)

even though it would otherwise be compatible.

This issue also looks like it applies to the map_data method although I have not tested it.

[aaronayres35]

#802 and #804 have been merged. This PR can be closed after we go through and audit the code base to see if anything was missed

[rahulporuri]

This PR can be closed after we go through and audit the code base to see if anything was missed

I'm guessing you meant "this issue can be closed". Can you do the audit @aaronayres35 ?

[aaronayres35]

So I've gone through the code base and I think it looks like we've got everything.

The only thing that stood out was the map_screen method on DataView which I am not sure why it exists. DataView has a class hierarchy which traces back to enable Container and none of its base classes define / use a map_screen method. It also isn't used anywhere in DataView itself, or in Plot which subclasses DataView. In other words I think it is completely unused. It currently is similar to what BaseXYPlot.map_screen was previously. I was thinking to update it to match, but after looking closer I think it can maybe just be removed entirely? It is technically public api though so maybe we shouldn't do that. I don't know when one would be using the method on the DataView/Plot rather than the plot renderer though...

@rahulporuri what do you think?

[rahulporuri]

Nice catch. I'll try to look into the history of why that method was added - but it looks like we can close this issue and open another one regarding deprecating and potentially removing that method.

[rahulporuri]

so it looks like the map_data and map_screen methods were added to the DataView here - https://github.com/enthought/chaco/commit/1f5f5c4b8eb0e0a05c4476dd98bde2df7e5c1873. It isn't clear why though.