Skip to content

Partial Revert "ENH: infer freq in timedelta_range (#32377)" #36595

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.

Sign up for GitHub

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

Jump to bottom
Merged
jreback merged 10 commits into from
Sep 26, 2020
Merged

Partial Revert "ENH: infer freq in timedelta_range (#32377)" #36595

jreback merged 10 commits into from
Sep 26, 2020

Conversation

@simonjayhawkins
Copy link
Member

@simonjayhawkins simonjayhawkins commented Sep 24, 2020 •
edited
Loading

closes #35897

maybe alternative to #36582 if no changes since #32377 now fail (not tested locally)

cc @phofl @jbrockmendel

@simonjayhawkins simonjayhawkins added this to the 1.1.3 milestone Sep 24, 2020
@simonjayhawkins simonjayhawkins added Timedelta Timedelta data type Regression Functionality that used to work in a prior pandas version labels Sep 24, 2020
@simonjayhawkins
Copy link
Member Author

simonjayhawkins commented Sep 24, 2020

maybe alternative to #36582 if no changes since #32377 now fail (not tested locally)

looks like maybe just a doctest added since.

the release note will need to explain that the enhancement has been reverted, so a proper fix maybe more preferable.

will leave this a draft for now.

@jbrockmendel
Copy link
Member

jbrockmendel commented Sep 24, 2020

LGTM

@jreback
Copy link
Contributor

jreback commented Sep 24, 2020

@simonjayhawkins yep looks good here too; if you can add a release note and merge master; ping on green (and you are in draft status)

@jbrockmendel jbrockmendel mentioned this pull request Sep 25, 2020
Merged
5 tasks
@jorisvandenbossche jorisvandenbossche modified the milestones: 1.1.3, 1.2 Sep 25, 2020
@pep8speaks
Copy link

pep8speaks commented Sep 25, 2020 •
edited
Loading

Hello @simonjayhawkins! 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-09-25 18:36:20 UTC

@simonjayhawkins simonjayhawkins marked this pull request as ready for review September 25, 2020 10:37
@simonjayhawkins simonjayhawkins mentioned this pull request Sep 25, 2020
Open
@simonjayhawkins
Copy link
Member Author

simonjayhawkins commented Sep 25, 2020

ci restarted

=========================== short test summary info ===========================
FAILED pandas/tests/plotting/test_groupby.py::TestDataFrameGroupByPlots::test_series_groupby_plotting_nominally_works
= 1 failed, 75523 passed, 1809 skipped, 1165 xfailed, 67 warnings in 1223.71s (0:20:23) =

@simonjayhawkins
Copy link
Member Author

simonjayhawkins commented Sep 25, 2020

@simonjayhawkins yep looks good here too; if you can add a release note and merge master; ping on green (and you are in draft status)

@jreback green

jbrockmendel
jbrockmendel reviewed Sep 25, 2020
View reviewed changes
pandas/core/arrays/timedeltas.py
if freq.nanos == 0 and self.freq.nanos != 0:
# e.g. if self.freq is Nano(1) then dividing by 2
# rounds down to zero
freq = None
Copy link
Member

@jbrockmendel jbrockmendel Sep 25, 2020

Choose a reason for hiding this comment

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

is reverting this necessary? im surprised there isnt a test that hits it

Copy link
Member Author

@simonjayhawkins simonjayhawkins Sep 25, 2020

Choose a reason for hiding this comment

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

no idea. maybe added to stop failing tests with inferred freq?

@jreback jreback merged commit 8142651 into pandas-dev:master Sep 26, 2020
@jreback
Copy link
Contributor

jreback commented Sep 26, 2020

3rd time the charm, thanks @simonjayhawkins

@simonjayhawkins simonjayhawkins deleted the revert-infer branch September 26, 2020 08:35
@simonjayhawkins
Copy link
Member Author

simonjayhawkins commented Sep 26, 2020

@meeseeksdev backport 1.1.x

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Sep 26, 2020
@simonjayhawkins @meeseeksmachine
Backport PR pandas-dev#36595: Partial Revert "ENH: infer freq in time…
020db8d 
…delta_range (pandas-dev#32377)"
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 26, 2020
Merged
simonjayhawkins added a commit that referenced this pull request Sep 26, 2020
@meeseeksmachine @simonjayhawkins
Backport PR #36595: Partial Revert "ENH: infer freq in timedelta_range (
f953112 
#32377)" (#36659)

Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
@simonjayhawkins
Partial Revert "ENH: infer freq in timedelta_range (pandas-dev#32377)" (
d60bea8 
pandas-dev#36595)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@jreback jreback Awaiting requested review from jreback

Assignees

No one assigned

Labels

Regression Functionality that used to work in a prior pandas version Timedelta Timedelta data type

Projects

None yet

Milestone

1.1.3

Development

Successfully merging this pull request may close these issues.

Regression in pandas 1.1.0: no longer allows adding a timestamp to timedelta range to create a datetime range.

5 participants

@simonjayhawkins @jbrockmendel @jreback @pep8speaks @jorisvandenbossche