Skip to content

Fix tuple markers#16770

Merged
tacaswell merged 4 commits intomatplotlib:masterfrom
QuLogic:fix-markers
Mar 16, 2020
Merged

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

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 14, 2020

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 QuLogic added this to the v3.2.1 milestone Mar 14, 2020
@QuLogic
Copy link
Member Author

QuLogic commented Mar 14, 2020

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

@QuLogic 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
Copy link
Member Author

QuLogic commented Mar 14, 2020

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

@tacaswell
Copy link
Member

tacaswell commented Mar 15, 2020

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

@tacaswell
Copy link
Member

tacaswell commented Mar 15, 2020 •
edited
Loading

How did you find this out of curiosity?

@QuLogic
Copy link
Member Author

QuLogic commented Mar 15, 2020

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

@tacaswell
Copy link
Member

tacaswell commented Mar 15, 2020

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

@QuLogic
Copy link
Member Author

QuLogic commented Mar 15, 2020

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

tacaswell commented Mar 15, 2020

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

@tacaswell 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
@tacaswell @meeseeksmachine
Backport PR matplotlib#16770: Fix tuple markers
dc81103
@meeseeksmachine meeseeksmachine mentioned this pull request Mar 16, 2020
Merged
@QuLogic QuLogic deleted the fix-markers branch March 16, 2020 19:13
@QuLogic QuLogic mentioned this pull request Mar 16, 2020
Merged
2 tasks
QuLogic added a commit that referenced this pull request Mar 17, 2020
@QuLogic
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 QuLogic mentioned this pull request Mar 17, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@timhoffm 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

@QuLogic @tacaswell @timhoffm