Cleanup map_screen return types#726
Conversation
aaronayres35
changed the title
This reverts commit 9e219a0.
|
I think this may be a bit strict on the expected shapes, but my brain isn't in chaco mode right now. For example, I seem to recall that the implicit assumption of a BaseXYPlot map method is that the shape is (..., 2) (ie. we only make assumptions about the size of the last dimension. |
That makes sense. I was having to do special things with segment plot as it was expecting to work with shapes of Nx2x2, not Nx2. I will rework to get it more general to (..., 2) |
| def map_screen(self, data_array): | ||
| """Maps an Nx2x2 array of data points into screen space and returns it | ||
| as an array. | ||
|
|
||
| Implements the AbstractPlotRenderer interface. | ||
| """ | ||
| # ensure data_array is an Nx2x2 ndarray | ||
| data_array = np.asarray(data_array) | ||
| data_array = data_array.reshape(-1, 2, 2) | ||
|
|
||
| if len(data_array) == 0: | ||
| return np.empty(shape=(0, 2, 2)) | ||
|
|
||
| x_ary, y_ary = np.transpose(data_array) | ||
|
|
||
| sx = self.index_mapper.map_screen(x_ary) | ||
| sy = self.value_mapper.map_screen(y_ary) | ||
| if self.orientation == "h": | ||
| return np.transpose(np.array((sx, sy))) | ||
| else: | ||
| return np.transpose(np.array((sy, sx))) | ||
|
|
There was a problem hiding this comment.
Note this is effectively exactly the same as BaseXYPlot.map_screen only here we enforce Nx2x2. Previously this worked with BaseXYPlot.map_screen as the reshape line wasn't there to enforce Nx2 (hence Corran's comment). However, in the BaseXYPlot there was a comment explicitly saying "data array is Nx2 array" but that wasn't actually what the code assumed.
To avoid potential breaks we probably don't want to make that change to BaseXYPlot.
But I don't see a problem with being more specific on subclasses. ie I think it makes sense for SegmentPlot to enforce an Nx2x2 data array? That's what it wants to be working with anyway.
BaseXYPlot should also be made to be explicit about the input shape. Even if it is (..., 2) that should be clearly documented / enforced.
aaronayres35
changed the title
|
I've pulled out parts of this PR into standalone PRs #802 #803 #804 I will eventually open 1 last PR to pull out all the more general changes of this PR which weren't specific to an issue, but rather were about general consistency across the codebase. Once that is opened and the new PRs merged we can close this PR. |
|
This PR shouldn't be merged. I merged master into this branch basically just to check what changes still need to be pulled out into stand alone PRs. |
| data_pts = array(data_pts) | ||
| data_pts = data_pts.reshape(-1, 2) |
There was a problem hiding this comment.
the column_stack call below actually avoids any errors here making this code un-necessary. Ref:
chaco/chaco/tests/test_grid_mapper.py
Lines 36 to 42 in d83e4b8
| # ensure data_array is an Nx2 ndarray | ||
| data_array = array(data_array) | ||
| data_array = data_array.reshape(-1,2) |
There was a problem hiding this comment.
these are actually redundant / unneeded as in the return statement we call array
|
Closing as the content of this PR has been pulled out into smaller PRs |
2 participants
fixes #272, #289, #550
This PR goes through the full chaco code base and makes sure definitions of
map_screenalways return ndarrays of a consistent shape.To do so, it also converts inputs to an ndarray of an appropriate shape. For exampleBaseXYPlot.map_screenshould have 2-dim data points, so it converts its input into Nx2 array.This caused some problem with segment plot which actually keeps its data as Nx2x2 array where a "data point" is really 2 points representing a segment. To address this I had SegmentPlot overwrite its map_screen method.Currently the first commit has all the "real" changes, and it may be easier for a reviewer to look there specifically first. The second commit simply makes clean up changes to use numpy as np everywhere. I had been doing so in my changes, but the original code was importing specific things from numpy. I made them consistent by just using np everywhere.This PR is a WIP as it still needs to add extensive testsI've added various testsSimilar problems exist with
map_data, for example see: #542This PR does a couple things. First, in the case where the given input to
map_screenis empty, it will now return an empty ndarray or appropriate shape rather than an empty list. Additionally, inputs to map_screen are checked / forced to be the arrays of correct shape. In some cases this is Nx2, others it is Nx2x2, others it is Nx1. These shapes were commented on before (and at times assumed) but not enforced. This change prevents strange bugs that would result from passing in a single point which would have shape (2,) and then calling transpose and unpacking the result leading to working with individual floating point values rather than singleton arrays. As per comment below, BaseXYPlot now converts the data to an array, checks if it has ndim == 1 in which case it reshapes it to (-1,2) (handling the case just described), and then checks if the shape ends with 2. If it does not a Value Error is raised as the ensuing calculations assume this to be the case.In other cases we can be strict in expecting Nx2 or Nx2x2 for example, eg Barplot or SegmentPlot.