8000 Fix barbs to accept array of bool for `flip_barb` by dopplershift · Pull Request #14296 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Fix barbs to accept array of bool for flip_barb #14296

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2019
Merged

Fix barbs to accept array of bool for flip_barb #14296

merged 1 commit into from
Jun 3, 2019

Conversation

dopplershift
Copy link
Contributor

Original docstring for barbs() states that it should accept an array of
bool for flip_barb, to allow flipping individual barbs for a field.
Apparently, this has never worked--this commit fixes that. This
is a very important meteorological use case, since barbs should flip in
the southern hemisphere.

PR Summary

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

@dopplershift dopplershift added this to the v3.1.1 milestone May 21, 2019
@dopplershift dopplershift mentioned this pull request May 21, 2019
Merged
@dopplershift dopplershift force-pushed the fix-barb-flip branch 3 times, most recently from 0b60674 to 4c6fb47 Compare May 22, 2019 21:49
@dopplershift
Fix barbs to accept array of bool for flip_barb
c87aea3 
Original docstring for barbs() states that it should accept an array of
bool for flip_barb, to allow flipping individual barbs for a field.
Apparently, this has never worked--this commit fixes that. This
is a very important meteorological use case, since barbs should flip in
the southern hemisphere.
@efiring efiring requested a review from WeatherGod May 24, 2019 23:17
timhoffm
timhoffm reviewed May 25, 2019
View reviewed changes
Copy link
Member
@timhoffm timhoffm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add a test for a scalar flip_barb as well. The broadcasting is non-trivial and should be tested.

@dopplershift
Copy link
Contributor Author

Technically, the other tests use the default, which is True. Worth adding one for False?

@timhoffm
Copy link
Member

Huh? I think the default is False. And executing the flipping logic for the scalar True would be good because the broadcasting is non-trivial.

tacaswell
tacaswell approved these changes Jun 3, 2019
View reviewed changes
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.

We do have a test of the flip_barb=False case (as that is the default). I don't think adding a case of testing flip_barb=True will give us anything that we don't already have (either for the scalar case are showing that barbs can flip).

@timhoffm timhoffm merged commit e74800a into matplotlib:master Jun 3, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 3, 2019
@timhoffm
Backport PR matplotlib#14296: Fix barbs to accept array of bool for `…
10a01ea 
…flip_barb`
@meeseeksmachine meeseeksmachine mentioned this pull request Jun 3, 2019
Merged
tacaswell added a commit that referenced this pull request Jun 10, 2019
@tacaswell
Merge pull request #14434 from meeseeksmachine/auto-backport-of-pr-14…
dfc8dbd 
…296-on-v3.1.x

Backport PR #14296 on branch v3.1.x (Fix barbs to accept array of bool for `flip_barb`)
@dopplershift dopplershift deleted the fix-barb-flip branch July 1, 2020 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm left review comments

@efiring efiring efiring approved these changes

@tacaswell tacaswell tacaswell approved these changes

@WeatherGod WeatherGod Awaiting requested review from WeatherGod

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@dopplershift @timhoffm @efiring @tacaswell
0