Skip to content
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

DEPR: tz kwarg in Period.to_timestamp #34522

Merged

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

inconsistent with PeriodArray/PeriodIndex methods

@jreback jreback added Deprecate Functionality to remove in pandas Period Period data type Timezones Timezone data dtype labels Jun 2, 2020
@jreback
Copy link
Contributor

jreback commented Jun 2, 2020

pls add this to the deprecation removal issue

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Is there a motivation?

@jbrockmendel
Copy link
Member Author

Is there a motivation?

As mentioned in the OP, its inconsistent with the PeriodArray/PeriodIndex methods.

@jorisvandenbossche
Copy link
Member

Whoops, sorry, missed that sentence.

Now, this keyword doesn't seem really a burden to me, so I still don't find that a fully convincing reason (could it be added to the array method?)

Also, it seems we are using this feature internally in pandas, according to the test failure.

@jorisvandenbossche jorisvandenbossche changed the title DEPR: tz kwarg in Period.tz_convert DEPR: tz kwarg in Period.tz_timestamp Jun 2, 2020
@jorisvandenbossche jorisvandenbossche changed the title DEPR: tz kwarg in Period.tz_timestamp DEPR: tz kwarg in Period.to_timestamp Jun 2, 2020
@jorisvandenbossche
Copy link
Member

Also, it seems we are using this feature internally in pandas, according to the test failure.

It's in timeseries.py. It can of course easily be fixed there to use tz_localize

@jbrockmendel
Copy link
Member Author

Now, this keyword doesn't seem really a burden to me, so I still don't find that a fully convincing reason (could it be added to the array method?)

Either way works. My default is to prefer a smaller API surface, but not a strong preferenc.

Note also that Period.to_timestamp incorrectly ignores the tz arg in the how == 'end' case, so i see it as deprecating a broken feature.

@jreback jreback added this to the 1.1 milestone Jun 2, 2020
@jreback
Copy link
Contributor

jreback commented Jun 2, 2020

agree with @jbrockmendel here; simpler is better, we always convert to naive, which is inline the model of Periods which are naive by definition.

@jorisvandenbossche
Copy link
Member

OK, if it's only partly implemented, that's certainly another reason to deprecate it

@jorisvandenbossche jorisvandenbossche merged commit e79487d into pandas-dev:master Jun 3, 2020
@jbrockmendel jbrockmendel deleted the depr-to_timestamp-tz branch June 3, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Period Period data type Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants