Skip to content
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.

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

Line block tests #29

Merged
merged 9 commits into from Feb 7, 2019
Merged

Line block tests #29

merged 9 commits into from Feb 7, 2019

Conversation

TomNicholas
Copy link
Contributor

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()?

@t-makaro
Copy link
Owner

t-makaro commented Feb 2, 2019

+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 np.asanyarray() is good because if this is a list of arrays of the same length then it will be a 2D array, and if they are not the same length then it is a 1D array with dtype=object, which is easily recognized by the _make_slice() method.

I'll take a deeper look at the logic later, but it looks good.

@t-makaro t-makaro added this to the 0.4 milestone Feb 2, 2019
@t-makaro t-makaro self-requested a review February 3, 2019 00:56
Copy link
Owner

@t-makaro t-makaro left a comment

Choose a reason for hiding this comment

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

I believe that you'll need to rebase to avoid merge conflicts with #31.

tests/test_blocks.py Show resolved Hide resolved
tests/test_blocks.py Show resolved Hide resolved
tests/test_blocks.py Show resolved Hide resolved
if y is None:
raise ValueError("Must supply y data to plot")
y = np.asanyarray(y)
if y.ndim != 2:
Copy link
Owner

Choose a reason for hiding this comment

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

A list of non-uniform length arrays will fail this condition.

@TomNicholas
Copy link
Contributor Author

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:

AssertionError: 
E       Arrays are not equal
E       
E       (mismatch 100.0%)
E        x: array([array([1, 2, 3]), array([1, 2, 3, 4])], dtype=object)
E        y: array([array([1, 2, 3]), array([1, 2, 3, 4])], dtype=object)

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 .is_list should be renamed, because they don't store lists at all!)

Copy link
Owner

@t-makaro t-makaro left a comment

Choose a reason for hiding this comment

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

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`.
Copy link
Owner

Choose a reason for hiding this comment

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

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:
Copy link
Owner

Choose a reason for hiding this comment

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

I think this else is redundant (but not the self._is_list), since if the above error is thrown it doesn't matter.

animatplot/blocks/lineplots.py Show resolved Hide resolved
tests/test_blocks.py Outdated Show resolved Hide resolved
@t-makaro
Copy link
Owner

t-makaro commented Feb 7, 2019

Excellent work. Thank you! Merging!

@t-makaro t-makaro merged commit d86fa49 into t-makaro:master Feb 7, 2019
@TomNicholas TomNicholas deleted the line_block_tests branch February 7, 2019 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants