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

Allow numpy arrays in markevery #17276

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

ImportanceOfBeingErnest
Copy link
Member

PR Summary

This line

if self._markevery != every:

prevents numpy arrays to be used as input to markevery, Because The 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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.3.0 milestone Apr 30, 2020
@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the markevery-numpy branch 2 times, most recently from 9cc1cee to b90026f Compare April 30, 2020 16:21
@timhoffm
Copy link
Member

timhoffm commented May 2, 2020

test_lines/test_markevery[png]

got:
test_markevery png
expected:
test_markevery png -expected
diff:
test_markevery png -failed-diff

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the markevery-numpy branch 4 times, most recently from 5091cd3 to d047d13 Compare May 2, 2020 13:31
@ImportanceOfBeingErnest
Copy link
Member Author

Wow, this was hard - because the tests ran perfectly fine locally. Turns out I stepped directly into numpy/numpy#16023 (np.array(["0"]).astype(bool) is False in all but the very recent numpy 1.18.3).

t = np.linspace(0, 3, 14)
y = np.random.rand(len(t))

casesA = [None, 4, (2, 5), [1, 5, 11],
Copy link
Member

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))
Copy link
Member

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 use y = t**2 or any other simple function.

@efiring
Copy link
Member

efiring commented May 8, 2020

@timhoffm's suggested test changes look easy and worthwhile so I'm holding off on a review, but otherwise this looks good.

Copy link
Member

@tacaswell tacaswell left a 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.

Copy link
Member

@efiring efiring left a 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.

@efiring efiring merged commit e7d498c into matplotlib:master Jun 3, 2020
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

5 participants