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

API: treat xunits=None and yunits=None as "default" by tacaswell · Pull Request #17828 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

API: treat xunits=None and yunits=None as "default" - #17828

Merged
efiring merged 1 commit into
matplotlib:masterfrom
tacaswell:fix_none_units
Jul 5, 2020
ConversationCommits (1)ChecksFiles changed
Merged

API: treat xunits=None and yunits=None as "default"#17828
efiring merged 1 commit into
matplotlib:masterfrom
tacaswell:fix_none_units

Conversation

tacaswell commented Jul 3, 2020

Copy link
Copy Markdown
Member

PR Summary

If the user passes None, treat as if they had passed nothing and do
not try to change the units on the respective axis.

closes #11095

I still need to write an API changes note (even though this is fixing a regression, it is a very old regression).

I don't think this conflicts with #17824 as the improved error message is a good idea independent of if this goes in.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

tacaswell added this to the v3.4.0 milestone Jul 3, 2020
anntzer reviewed Jul 3, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_category.py Outdated

@pytest.mark.style('default')
@check_figures_equal(extensions=["png"])
def test_overliding_units(fig_test, fig_ref):

anntzer Jul 3, 2020

Copy link
Copy Markdown
Contributor

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

"overliding"?

tacaswell Jul 3, 2020

Copy link
Copy Markdown
Member Author

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

l and r are next to each other on my keyboard. I wish I could blame all of my miss-spellings on poor typing as easily as this one...

anntzer Jul 5, 2020

Copy link
Copy Markdown
Contributor

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

ah, dvorak? :)

story645 reviewed Jul 3, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_category.py
Comment on lines +278 to +284
from datetime import datetime

t0 = datetime(2018, 3, 1)
t1 = datetime(2018, 3, 2)
t2 = datetime(2018, 3, 3)
t3 = datetime(2018, 3, 4)

story645 Jul 3, 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

what's the point of adding in the datetime here if that's not what's being tested?

tacaswell Jul 3, 2020

Copy link
Copy Markdown
Member Author

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

It is a converter class that does not care about the value of Axis.units, but I think it is worth putting units on both Axis.

dstansby Jul 3, 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

Would it be better to test using some data that does care about/use units? I'm wondering if datetime always has the units set to None all the time anyway.

tacaswell Jul 3, 2020

Copy link
Copy Markdown
Member Author

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

The string catarogicals do use Axis.units to store critical state (which is what broke in #11095).

dstansby Jul 3, 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

Oh woops, I missed that categoricals were being plotted too

story645 Jul 5, 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

@tacaswell so that's what concerns me, that you're putting two potential points of failure in this test - when it fails will it be easy to see that it was the categoricals that broke this?

tacaswell Jul 5, 2020

Copy link
Copy Markdown
Member Author

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

All of our tests as pretty much closer to integration tests than unit tests. I am optimistic that if (when) this break who ever is debugging it will be able to sort out the root cause with out too much trouble.

tacaswell force-pushed the fix_none_units branch 2 times, most recently from 8649ad9 to f0851fc Compare July 3, 2020 16:56
tacaswell marked this pull request as ready for review July 3, 2020 16:56
dstansby reviewed Jul 3, 2020
View reviewed changes
Comment thread doc/api/next_api_changes/behavior/17828-TAC.rst Outdated
Comment thread lib/matplotlib/tests/test_category.py
Comment on lines +278 to +284
from datetime import datetime

t0 = datetime(2018, 3, 1)
t1 = datetime(2018, 3, 2)
t2 = datetime(2018, 3, 3)
t3 = datetime(2018, 3, 4)

dstansby Jul 3, 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

Would it be better to test using some data that does care about/use units? I'm wondering if datetime always has the units set to None all the time anyway.

tacaswell added API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … labels Jul 3, 2020
efiring requested changes Jul 4, 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

Just typos; otherwise looks fine.

Comment thread lib/matplotlib/axes/_base.py Outdated
.. warning ::

This method may mutate the dictionary passed in an kwargs and
the Axes instances attached to this Axes.

efiring Jul 4, 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

"passed in an kwargs" -> "passed in as kwargs"
"the Axes instances" -> "the Axis instances"

API: treat xunits=None and yunits=None as "default"
444eaa4 
If the user passes None, treat as if they had passed nothing and do
not try to change the units on the respective axis.

closes matplotlib#11095

Co-authored-by: David Stansby <dstansby@gmail.com>
efiring merged commit d266e71 into matplotlib:master Jul 5, 2020
tacaswell deleted the fix_none_units branch July 5, 2020 20:37
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

story645 story645 left review comments
anntzer anntzer left review comments
efiring efiring approved these changes
dstansby dstansby approved these changes

Assignees

No one assigned

Labels

API: changes Changes to the public API, typically requiring deprecation. API: consistency Consistency of the matplotlib API, including naming, behavior, defaults, … topic: units and array ducktypes

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

Repeated plot calls with xunits=None throws exception

5 participants

Back | FazBrowse Home | New Git URL