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

Fix tuple markers by QuLogic · Pull Request #16770 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix tuple markers - #16770

Merged
tacaswell merged 4 commits into
matplotlib:masterfrom
QuLogic:fix-markers
Mar 16, 2020
ConversationCommits (4)ChecksFiles changed
Merged

Fix tuple markers#16770
tacaswell merged 4 commits into
matplotlib:masterfrom
QuLogic:fix-markers

Conversation

QuLogic commented Mar 14, 2020

Copy link
Copy Markdown
Member

PR Summary

I was able to write a test for polygon tuple markers, but not for star or asterisk yet. The star tuple is not the same as * and asterisk, while similar to + or x, seem to have a different weight.

This broke in 3.2.0 in #14244.

PR Checklist

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

QuLogic added this to the v3.2.1 milestone Mar 14, 2020

QuLogic commented Mar 14, 2020

Copy link
Copy Markdown
Member Author

Oh, that was supposed to be WIP because the other two tests weren't written, but oh well.

QuLogic added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: work in progress labels Mar 14, 2020

QuLogic commented Mar 14, 2020

Copy link
Copy Markdown
Member Author

Not sure what to do about star markers, so I just did a smoke test.

tacaswell commented Mar 15, 2020

Copy link
Copy Markdown
Member

Specifically https://github.com/matplotlib/matplotlib/pull/14244/files#diff-f078f095bee3dd9c6ed756b47a04f74fL375

tacaswell commented Mar 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

How did you find this out of curiosity?

QuLogic commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

I was running flake8 without extra warnings and noticed an unused variable.

tacaswell commented Mar 15, 2020

Copy link
Copy Markdown
Member

Should L333 turn into an else? I think there is now a path through this for an unboundlocal error.

QuLogic commented Mar 15, 2020

Copy link
Copy Markdown
Member Author

That is verified in set_marker:

matplotlib/lib/matplotlib/markers.py

Lines 279 to 281 in e9da814

elif (isinstance(marker, Sized) and len(marker) in (2, 3) and
marker[1] in (0, 1, 2)):
self._marker_function = self._set_tuple_marker

tacaswell commented Mar 15, 2020

Copy link
Copy Markdown
Member

fair enough, still seems like a bit of a foot-cannon to leave in the code base.

tacaswell merged commit aa1ed3e into matplotlib:master Mar 16, 2020
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 16, 2020
Backport PR matplotlib#16770: Fix tuple markers
dc81103
meeseeksmachine mentioned this pull request Mar 16, 2020
Merged
QuLogic deleted the fix-markers branch March 16, 2020 19:13
QuLogic mentioned this pull request Mar 16, 2020
Merged
2 tasks done
QuLogic added a commit that referenced this pull request Mar 17, 2020
Merge pull request #16791 from meeseeksmachine/auto-backport-of-pr-16…
e935d85 
…770-on-v3.2.x

Backport PR #16770 on branch v3.2.x (Fix tuple markers)
QuLogic mentioned this pull request Mar 17, 2020
Merged
2 tasks done
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

tacaswell tacaswell approved these changes
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v3.2.1

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL