FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow numpy arrays in markevery by ImportanceOfBeingErnest · Pull Request #17276 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Allow numpy arrays in markevery - #17276

Merged
efiring merged 1 commit into
matplotlib:masterfrom
ImportanceOfBeingErnest:markevery-numpy
Jun 3, 2020
ConversationCommits (1)ChecksFiles changed
Merged

Allow numpy arrays in markevery#17276
efiring merged 1 commit into
matplotlib:masterfrom
ImportanceOfBeingErnest:markevery-numpy

Conversation

ImportanceOfBeingErnest commented Apr 30, 2020

Copy link
Copy Markdown
Member

PR Summary

This line

matplotlib/lib/matplotlib/lines.py

Line 583 in 9a9c8b8

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 added this to the v3.3.0 milestone Apr 30, 2020
ImportanceOfBeingErnest force-pushed the markevery-numpy branch 2 times, most recently from 9cc1cee to b90026f Compare April 30, 2020 16:21
timhoffm reviewed May 1, 2020
View reviewed changes
Comment thread lib/matplotlib/lines.py

timhoffm commented May 2, 2020

Copy link
Copy Markdown
Member

test_lines/test_markevery[png]

got:

expected:

diff:

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

ImportanceOfBeingErnest commented May 2, 2020

Copy link
Copy Markdown
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).

timhoffm reviewed May 2, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_lines.py
t = np.linspace(0, 3, 14)
y = np.random.rand(len(t))

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

timhoffm May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/matplotlib/tests/test_lines.py
def test_markevery(fig_test, fig_ref):
np.random.seed(42)
t = np.linspace(0, 3, 14)
y = np.random.rand(len(t))

timhoffm May 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Optional: There is no gain from random numbers here. You could just use y = t**2 or any other simple function.

efiring commented May 8, 2020

Copy link
Copy Markdown
Member

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

tacaswell approved these changes May 20, 2020
View reviewed changes

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

efiring approved these changes Jun 3, 2020
View reviewed changes

efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 merged commit e7d498c into matplotlib:master Jun 3, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic left review comments
timhoffm timhoffm left review comments
efiring efiring approved these changes
tacaswell tacaswell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL