Skip to content

API: make Day preserve time-of-day across DST transitions #55502

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

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Oct 12, 2023

Updated version of #51874.

I'm trying to find things to split off of this or do as bite-size deprecations. Not much optimism on those fronts. I expect we'll need to do this all-at-once in 3.0 or as a giant deprecation in 2.x with a future option.

update: ideas for bits to split off or deprecate

  • Day.__truediv__ and multiplication by non-int could be deprecated, this shouldn't be too painful for users since i doubt anyone actually uses it
  • deprecate allowing Timedelta(day_obj). Again should be rare
  • deprecate allowing Day objects for freq in TDI constructor

@mroeschke mroeschke added Frequency DateOffsets Timezones Timezone data dtype labels Oct 13, 2023
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Nov 15, 2023
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Nov 17, 2023

I'm trying to find things to split off of this or do as bite-size deprecations. Not much optimism on those fronts. I expect we'll need to do this all-at-once in 3.0 or as a giant deprecation in 2.x with a future option.

all at once sounds fine to be honest, I'd consider it a bug that

In [44]: pd.Series([datetime(2020, 10, 25)]).dt.tz_localize('Europe/London') + pd.tseries.offsets.Day(1)
Out[44]:
0   2020-10-25 23:00:00+00:00
dtype: datetime64[ns, Europe/London]

happens

Please do ping when it's ready for review (looks like there's lots of conflicts atm)

@jbrockmendel jbrockmendel requested a review from mroeschke as a code owner June 5, 2024 18:05
@jbrockmendel
Copy link
Member Author

@jorisvandenbossche can i get you to take a look here pre-merge (which is not imminent)?

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

I noticed there's quite a few places still where we convert Day to its hour equivalent and allow operations to still continue. IIRC back in the day I was hoping to converting Day to a non-fixed frequency and living with the consequences of Day and "D" having non-fixed frequency semantics throughout other operations (which I would imagine be for things to raise more)

Are the conversions of Day to hours needed for this change to be less breaking?

@jbrockmendel
Copy link
Member Author

Are the conversions of Day to hours needed for this change to be less breaking?

IIRC they were for either a) cases where we went DatetimeIndex->TimedeltaIndex (e.g. subtracting a scalar from DatetimeIndex) or b) to allow users to continue to pass freq="2D" to TimedeltaIndex-like functions. The latter could be disallowed either directly or via deprecation.

I'll take another pass and see if any of these usages can be removed.

@jbrockmendel
Copy link
Member Author

jbrockmendel commented Jul 28, 2025

On last week's dev call we discussed an option to allow TimedeltaIndex.freq to be a Day even though that isn't a Tick, the idea being that this would reduce churn for users. I've been working on a branch that does that, but still have 76 tests failing. Most of these seem to stem from the fact that Day(1) no longer compares as equal to Hour(24) (or Second(86400) or [...]).

e.g. in pandas/tests/resample/test_period_index.py::TestPeriodIndex::test_resample_with_offset (in main and the branch) result.index.freq is Day(1) while expected.index.freq is Hour(24). tm.assert_series_equal(result, expected) passes in main because they compare as equal.

One option is to patch the affected tests to say "close enough". I need to look more closely to determine if I'm comfortable with that. Update Very comfortable.

There's also a handful of resample tests (test_resample_origin_with_day_freq_on_dst) that look materially different that I don't have a good handle on.

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

Successfully merging this pull request may close these issues.

API: Make 'D' & offsets.Day always operate as calendar day instead of 24 Hour
3 participants