-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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.
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
BUG: no longer raise user warning when plotting tz aware time series #31207
BUG: no longer raise user warning when plotting tz aware time series #31207
Conversation
0ac99ae
to
ead2624
Compare
b283e9e
to
66adf3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely this was introduced in master and is not apparent on 0.25.3 ? if so we don't need a whatsnew note (@jorisvandenbossche )
data = data.to_period(freq=freq) | ||
with warnings.catch_warnings(): | ||
warnings.filterwarnings("ignore", category=UserWarning) | ||
data = data.to_period(freq=freq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't do this, instead I think just
data = data.tz_localize(None).to_period(freq=freq)
should suffice
66adf3b
to
4c23901
Compare
@MarcoGorelli tiny nitpicky workflow comment: can you use "BUG:" instead of "[BUG]" for PR titles? (just for consistency, that's how the majority of PRs do it) |
@@ -251,7 +251,7 @@ def _maybe_convert_index(ax, data): | |||
freq = frequencies.get_period_alias(freq) | |||
|
|||
if isinstance(data.index, ABCDatetimeIndex): | |||
data = data.to_period(freq=freq) | |||
data = data.tz_localize(None).to_period(freq=freq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To preserve the existing behaviour, this might need to be tz_convert
instead of tz_localize
? (the first will give the underlying UTC data, the other converts to naive local time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe check with a small actual example and see what plotting it gives (on 0.25.3 and on this PR).
Because intuitively, converting to local time zone sounds more useful, though. But not fully sure what the current behaviour is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcoGorelli can you try with something else as UTC? (as UTC is the one timezone where UTC and local is the same :-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorisvandenbossche sure :)
df = pd.DataFrame(
np.random.randn(100, 3),
index=pd.date_range(
"2012", freq="H", periods=100, tz=pytz.timezone("Africa/Gaborone")
),
columns=["a", "b", "c"],
)
df.head().plot()
It seems this is already happening in released pandas as well (checked 0.25.3) |
Can confirm this was already happening in 0.25.3. So, the whatsnew note is still needed? |
Yes, a release note will be needed if it's fixing something present in 0.25.3. Are we backporting this to 1.0.0? |
4c23901
to
7b504bf
Compare
@TomAugspurger sure, whatsnew entry added |
@MarcoGorelli can you merge master to fix the CI failure? If this is fixing a bug that was present in 0.25, then I think we should target 1.0.1 instead. |
Sure, I'll wait for the PR which creates that file to be merged |
7b504bf
to
55dacff
Compare
@TomAugspurger merged, whatsnew v1.0.1 note added |
@MarcoGorelli workflow related question: can you only push new commits? (instead of squashing/rebasing and force pushing) That makes it easier to see what changed in the GitHub interface |
It's not critical to backport this to 1.0.1, since it was already present in 0.25 and before. But it seems a safe fix ( |
tz = tz_aware_fixture | ||
index = date_range("1/1/2011", periods=2, freq="H", tz=tz) | ||
ts = Series([188.5, 328.25], index=index) | ||
_check_plot_works(ts.plot) | ||
with tm.assert_produces_warning(None): | ||
_check_plot_works(ts.plot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add something to check that the points are actually in the correct date-time, e.g. we removed the timezone (checking first/last prob enough)
I checked (visually) the output of the test function:
Is this right? Shouldn't both ticks be labeled? Anyway, this isn't affected by the current change (it's present on master as well) so I believe it would count as a separate issue, but have written the current test assuming it's expected output |
55dacff
to
1f1c20b
Compare
Hello @MarcoGorelli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-02-02 17:27:38 UTC |
|
f765a91
to
63af5bf
Compare
# Extract H:M component, check first point is in correct timezone. | ||
# NOTE: this test could be updated once GH 31548 is fixed, | ||
# so that the last point is checked as well. | ||
assert labels = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for debugging CI purposes
63af5bf
to
787ec3a
Compare
In one of the tests from the CI (linux py36_local), the axis labels are
I don't understand where that extra minute is coming from (nor can I understand why it only appears during CI), but I think it'll take me longer than the v1.0.1 deadline to figure this out. |
cc @TomAugspurger @jorisvandenbossche ok as is? |
@jreback Should I remove the test that checks the labels (which fails during CI) and open a separate issue about that? |
yes |
…nings, remove whatsnewentry
787ec3a
to
ecd4abb
Compare
…and tz_localize was previously approved
Thanks @MarcoGorelli ! |
…plotting tz aware time series
…z aware time series (#31601) Co-authored-by: Marco Gorelli <[email protected]>
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff