Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Allow numpy arrays in markevery#17276
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9cc1cee tob90026fCompareUh oh!
There was an error while loading.Please reload this page.
b90026f to808f7f4Compare
got: |
5091cd3 tod047d13Compared047d13 tob73d805CompareWow, this was hard - because the tests ran perfectly fine locally. Turns out I stepped directly intonumpy/numpy#16023 ( |
| t = np.linspace(0, 3, 14) | ||
| y = np.random.rand(len(t)) | ||
| casesA = [None, 4, (2, 5), [1, 5, 11], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this would be more readable:
cases = [ (None, "11111111111111"), (4, "10001000100010"), ... ] for ax_test, ax_ref, (markevery, pattern) in zip(axs_test, axs_ref, cases): ax_test.plot(t, y, "-gD", markevery=markevery) ax_ref.plot(t, y, "-gD", markevery=np.array(list(pattern)).astype(int).astype(bool))| def test_markevery(fig_test, fig_ref): | ||
| np.random.seed(42) | ||
| t = np.linspace(0, 3, 14) | ||
| y = np.random.rand(len(t)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Optional: There is no gain from random numbers here. You could just usey = t**2 or any other simple function.
@timhoffm's suggested test changes look easy and worthwhile so I'm holding off on a review, but otherwise this looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree with@timhoffm that grouping the cases the other way would be clearer, but I am not going to hold up merging over it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's get this in now.@timhoffm, if you would like to put in a simple subsequent PR to improve the tests, I'm sure that can also go in quickly.
PR Summary
This line
matplotlib/lib/matplotlib/lines.py
Line 583 in9a9c8b8
prevents numpy arrays to be used as input to
markevery, BecauseThe truth value of an array with more than one element is ambiguous.That's actually a regression that came in via#4091.
I think we would like to support numpy arrays wherever possible. So this PR just removes the check and sets the artist stale on every call to
set_markevery.Also creates tests for all possible cases that markevery can take.
PR Checklist