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: Deprecate tshift and integrate it to shift #34545

Merged
merged 8 commits into from
Jun 15, 2020

Conversation

fujiaxiang
Copy link
Member

@fujiaxiang fujiaxiang changed the title CLN: Deprecate tshift and integrated it to shift DEPR: Deprecate tshift and integrated it to shift Jun 3, 2020
@fujiaxiang fujiaxiang changed the title DEPR: Deprecate tshift and integrated it to shift DEPR: Deprecate tshift and integrate it to shift Jun 4, 2020
@jreback jreback added Deprecate Functionality to remove in pandas Datetime Datetime data dtype labels Jun 4, 2020
@jreback
Copy link
Contributor

jreback commented Jun 4, 2020

thanks. would not be averse to actually re-writing tshift in terms of shift; I think this would be straightforward. Also could merge this and then followon to do that.

@fujiaxiang
Copy link
Member Author

fujiaxiang commented Jun 4, 2020

would not be averse to actually re-writing tshift in terms of shift; I think this would be straightforward

Agree it's rather straightforward, but just to clarify a few things:

  1. we are still going to deprecate tshift after the re-write right? In other words, you don't mean to re-write and keep tshift right?
  2. do you think it's better to move the current _tshift inside shift? i.e. we won't have a separate method _tshift, but unpack what's in it and put inside shift

@jreback
Copy link
Contributor

jreback commented Jun 4, 2020

would not be averse to actually re-writing tshift in terms of shift; I think this would be straightforward

Agree it's rather straightforward, but just to clarify a few things:

  1. we are still going to deprecate tshift after the re-write right? In other words, you don't mean to re-write and keep tshift right?

correct, still deprecate

  1. do you think it's better to move the current _tshift inside shift? i.e. we won't have a separate method _tshift, but unpack what's in it and put inside shift

yes if possible (ideally simplified as much as possible).

@fujiaxiang
Copy link
Member Author

Have unpacked previous _tshift and moved into shift, also simplified the code a little bit. The deprecating tshift now calls shift. @jreback can you review?

@fujiaxiang fujiaxiang requested a review from jreback June 5, 2020 03:46
@jreback jreback added this to the 1.1 milestone Jun 9, 2020
@jreback
Copy link
Contributor

jreback commented Jun 14, 2020

@fujiaxiang can you rebase and some small comments, ping on green.

@fujiaxiang
Copy link
Member Author

@jreback have split up the tests into PI and DTI and also separated the failure tests
I tried to parametrize the test cases into PI and DTI but I realize they only have a very small proportion of code in common, so I decided to separate them into different tests

@jreback
Copy link
Contributor

jreback commented Jun 15, 2020

@jreback have split up the tests into PI and DTI and also separated the failure tests
I tried to parametrize the test cases into PI and DTI but I realize they only have a very small proportion of code in common, so I decided to separate them into different tests

kk sounds good. ping on green (note that there maybe be a failure on master that is unrelated)

@jreback jreback merged commit 09bdcbb into pandas-dev:master Jun 15, 2020
@jreback
Copy link
Contributor

jreback commented Jun 15, 2020

thanks @fujiaxiang very nice

@jorisvandenbossche jorisvandenbossche mentioned this pull request Sep 17, 2020
34 tasks
@fujiaxiang fujiaxiang deleted the deprecate_tshift branch March 20, 2021 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLN: tshift & shift could be consolidated
2 participants