Line block tests#29
Conversation
|
+1 to allowing 1D x-data to be passed. This is API breaking through since x, y are no longer accessible by keyword argument. I'm not saying to change that (I believe this is what matplotlib does, and following this convention in other blocks may make it easier to not pass x values), but it'll have to be noted in the changelog. Yes, I'd like to keep the ability to pass a list of arrays, for the purpose of animating changing line length. Converting the list of arrays with I'll take a deeper look at the logic later, but it looks good. |
| if y is None: | ||
| raise ValueError("Must supply y data to plot") | ||
| y = np.asanyarray(y) | ||
| if y.ndim != 2: |
There was a problem hiding this comment.
A list of non-uniform length arrays will fail this condition.
|
So I fixed the mistakes you spotted, and tried to add support for ragged arrays (numpy object arrays of arrays of various lengths). The code produces the right output for every test, but one of the tests fails because a numpy's assert functions can't seem to deal with an array of arrays: I've marked that test as xfailing for now, but I'm not sure what to do about this. We could probably just extract the values from the object array to check explicitly, but this weird behaviour might also be an indication that using array's of arrays is an abuse of numpy... (Also a minor point, I don't think the private attribute of blocks |
t-makaro
left a comment
There was a problem hiding this comment.
Only a few small changes left.
I don't think the private attribute of blocks .is_list should be renamed, because they don't store lists at all!
A ragged array behaves like a list (without an easy ability to append) but with some extra attributes. I don't think it's a big deal since it is internal, and the information is passed in as a list.
| :meth:`matplotlib.axes.Axes.plot` | ||
| This block animates a single line - to animate multiple lines you must call | ||
| this once for each line, and then animate all of the blocks returned by | ||
| passing a list of those blocks to `animatplot.animation.Animation`. |
There was a problem hiding this comment.
Public API is technically animatplot.Animation since animatplot.__init__ imports Animation and this is what the docs list.
| if not all(len(xline) == len(yline) for xline, yline in zip(x, y)): | ||
| raise ValueError("Length of x & y data must match one another " | ||
| "for every frame") | ||
| else: |
There was a problem hiding this comment.
I think this else is redundant (but not the self._is_list), since if the above error is thrown it doesn't matter.
|
Excellent work. Thank you! Merging! |
I added some unit tests for
blocks.Line(), and extended it to also accept a 1D array input for x (the most natural format for use in xarray).I also tried to clarify the logic for parsing the input, and make it raise more informative errors.
One high-level question - why allow users to input a list of numpy arrays at all? It makes the internal logic more complex and I don't really see what the benefit is. The only time a list of numpy arrays is a more suitable representation of data than a numpy array with an extra dimension is if the length of the array changes over time, but I don't know if that's actually supported by the plotting functions? Also if you do want to support do you want to keep the data internally as a list of arrays? Or is it okay to do what I did here and convert all lists of arrays straight to numpy arrays using
np.asanyarray()?