Skip to content

BUG: DataFrame/Series.tz_convert does not modifies original data with copy=False #24657

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

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

mroeschke
Copy link
Member

This also impacted tz_localize as well. Non blocking for 0.24

@codecov
Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #24657 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24657      +/-   ##
==========================================
- Coverage   92.37%   92.37%   -0.01%     
==========================================
  Files         166      166              
  Lines       52384    52384              
==========================================
- Hits        48391    48390       -1     
- Misses       3993     3994       +1
Flag Coverage Δ
#multiple 90.8% <100%> (ø) ⬆️
#single 43.02% <0%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.62% <100%> (ø) ⬆️
pandas/util/testing.py 88% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4994ca...c7bfdab. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #24657 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24657      +/-   ##
==========================================
- Coverage   92.37%   92.37%   -0.01%     
==========================================
  Files         166      166              
  Lines       52384    52384              
==========================================
- Hits        48391    48390       -1     
- Misses       3993     3994       +1
Flag Coverage Δ
#multiple 90.8% <100%> (ø) ⬆️
#single 43.02% <0%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.62% <100%> (ø) ⬆️
pandas/util/testing.py 88% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4994ca...c7bfdab. Read the comment docs.

@@ -9226,7 +9226,7 @@ def _tz_convert(ax, tz):
ax = _tz_convert(ax, tz)

result = self._constructor(self._data, copy=copy)
result.set_axis(ax, axis=axis, inplace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should really have the inplace arg rather than copy as that is the meaning here.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we should change this api though as don’t really love inplace generally

@gfyoung gfyoung added Bug Timezones Timezone data dtype labels Jan 7, 2019
@TomAugspurger TomAugspurger added this to the 0.24.0 milestone Jan 7, 2019
@TomAugspurger
Copy link
Contributor

@jreback were your comments from
#24657 (review) blocking for this PR? They seem out of scope for the bug fix here.

@jreback
Copy link
Contributor

jreback commented Jan 7, 2019

this fixes a bug but i think we ought to consider deprecating copy and making it inplace
even though i don’t like that

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jan 7, 2019 via email

@mroeschke
Copy link
Member Author

Well the order of operations could be:

  1. Fix this bug first
  2. Follow up PR with deprecating copy kwarg in tz_convert (or vice versa)?

@jreback
Copy link
Contributor

jreback commented Jan 7, 2019

that’s fine

@mroeschke mroeschke merged commit be406f3 into pandas-dev:master Jan 8, 2019
@mroeschke mroeschke deleted the tz_convert_copy branch January 8, 2019 00:29
@mroeschke
Copy link
Member Author

Thanks! Made #24667 as a follow up to deprecate copy in favor of inplace

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
… copy=False (pandas-dev#24657)

* BUG: DataFrame/Series.tz_localize/convert with copy=False does not mutate argument inplace

* Put back space
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
… copy=False (pandas-dev#24657)

* BUG: DataFrame/Series.tz_localize/convert with copy=False does not mutate argument inplace

* Put back space
result = Series(np.arange(0, 5),
index=date_range('20131027', periods=5, freq='1H',
tz=tz))
getattr(result, method)('UTC', copy=copy)
Copy link
Member

Choose a reason for hiding this comment

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

@mroeschke i'm confused by this; wouldn't we expect the tz in expected.index to be UTC, not tz

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming of the test is a bit confusing, but this call here should not be an inplace operation.

I'm checking that the original data result was not modified after calling tz_localize/tz_convert with copy

#6326 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

ahh i see. ill update #37498 appropriately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame/Series.tz_convert with copy=False modifies original data
5 participants