8000 Fix `set_ylim` unit handling by dopplershift · Pull Request #12468 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Fix set_ylim unit handling #12468

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 2 commits into from
Oct 10, 2018
Merged

Fix set_ylim unit handling #12468

merged 2 commits into from
Oct 10, 2018

Conversation

dopplershift
Copy link
Contributor

PR Summary

Code is missing a call to _process_unit_info (which is present in set_xlim). Without it, trying to call set_ylim on a blank plot using unit values results in the unit converter machinery being called to convert a value of None.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant

8000
@dopplershift
Refactor tests for units
06a8106 
Allows re-using the code setting up a unit converter interface for
Quantity.
@dopplershift
Fix missing call to _process_unit_info in set_ylim
02a9544 
Without it, calling set_ylim using unit support on a blank plot results
in trying to convert None.
@dopplershift dopplershift added this to the v3.0.x milestone Oct 9, 2018
@dopplershift dopplershift mentioned this pull request Oct 9, 2018
Closed
@jklymak
Copy link
Member
jklymak commented Oct 9, 2018

Does this affect #12198?

jklymak
jklymak approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

This looks good to me. Certainly an oversight that we weren't doing this the same as xlim...

@WeatherGod
Copy link
Member
WeatherGod commented Oct 10, 2018 via email

@dopplershift
Copy link
Contributor Author

Only definition of a function set_ylim in the code base is in _base.py, so I think we're good.

@dopplershift
Copy link
Contributor Author

Ah! But you have set_[x|y|z]lim3d, and the code is lurking in lib/mpl_toolkits. However...the code there is correct and all 3 methods have _process_unit_info being called.

@timhoffm timhoffm merged commit e08d7ba into matplotlib:master Oct 10, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 10, 2018
@dopplershift
Backport PR matplotlib#12468: Fix set_ylim unit handling
28bc96f
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 10, 2018
Merged
jklymak added a commit that referenced this pull request Oct 10, 2018
@jklymak
Merge pull request #12471 from meeseeksmachine/auto-backport-of-pr-12…
883005b 
…468-on-v3.0.x

Backport PR #12468 on branch v3.0.x (Fix `set_ylim` unit handling)
@dopplershift dopplershift deleted the fix-ylim-units branch October 10, 2018 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: units and array ducktypes
Projects
None yet
Milestone
v3.0.1
Development

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