API: treat xunits=None and yunits=None as "default" #17828
Conversation
|
|
||
| @pytest.mark.style('default') | ||
| @check_figures_equal(extensions=["png"]) | ||
| def test_overliding_units(fig_test, fig_ref): |
There was a problem hiding this comment.
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...
| from datetime import datetime | ||
|
|
||
| t0 = datetime(2018, 3, 1) | ||
| t1 = datetime(2018, 3, 2) | ||
| t2 = datetime(2018, 3, 3) | ||
| t3 = datetime(2018, 3, 4) | ||
|
|
There was a problem hiding this comment.
what's the point of adding in the datetime here if that's not what's being tested?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The string catarogicals do use Axis.units to store critical state (which is what broke in #11095).
There was a problem hiding this comment.
Oh woops, I missed that categoricals were being plotted too
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
| from datetime import datetime | ||
|
|
||
| t0 = datetime(2018, 3, 1) | ||
| t1 = datetime(2018, 3, 2) | ||
| t2 = datetime(2018, 3, 3) | ||
| t3 = datetime(2018, 3, 4) | ||
|
|
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
"passed in an kwargs" -> "passed in as kwargs"
"the Axes instances" -> "the Axis instances"
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>
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