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

WIP/API: Make Day not a Tick #51874

Closed
wants to merge 32 commits into from
Closed

Conversation

jbrockmendel
Copy link
Member

Still have a handful of failing tests and a bunch of ugly kludges.

With a second rc next week, we potentially have a second chance to get this in for 2.0. Largely posting this to get @mroeschke's thoughts on if it is worth trying to get this in.

@mroeschke
Copy link
Member

On first glance, I am a little surprised how much isinstance(offset, Day) special casing we still need, unless those are kludges (I would have hoped making it a calendar day would have provided some simplifications).

I am on the fence for rushing to get this into the second 2.0rc; personally I haven't fully comprehended all the implication of telling users to migrate from n * Days -> 24 * n Hours which seems like may touch a lot of areas I'm not aware of.

@mroeschke mroeschke added the Frequency DateOffsets label Mar 10, 2023
@jbrockmendel
Copy link
Member Author

More cleanup to do, but I'm more optimistic about this than I was a day or two ago.

Some of the diff will be reduced by separately addressing a pre-existing bug in resample incorrectly allowing non-Tick offsets in TimedeltaIndex.

@jbrockmendel
Copy link
Member Author

We could probably restore+deprecate Day multiplication and division, maybe also Timedelta(Day) and Day.nanos

@jbrockmendel
Copy link
Member Author

Some of this will be unnecessary with #51896

@jbrockmendel jbrockmendel mentioned this pull request Mar 13, 2023
1 task
@MarcoGorelli
Copy link
Member

+1 on the idea, but isn't it a bit big to get into the RC at this point?

@jbrockmendel
Copy link
Member Author

but isn't it a bit big to get into the RC at this point?

Yes, that's why I want buy-in!

This is a bug that @mroeschke and I have been trying to fix on and off since 2018. I only finally got a branch working a few days ago.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

This is a bug that @mroeschke and I have been trying to fix on and off since 2018. I only finally got a branch working a few days ago.

OK this makes me feel even more uneasy about rushing it in for 2.0

@mroeschke
Copy link
Member

Yeah while I want to see this fixed too, I also think this is a lot for the RC2 in a few days.

Additionally, maybe we can think of a more holistic change for 3.0 that may simplify this more; deprecating Ticks all together and introducing a CalendayDay frequency.

@jbrockmendel
Copy link
Member Author

Fair enough. I'll try to keep this green until the 3.0 merge window opens, maybe we can chip away at it with deprecations in the interim.

@jbrockmendel jbrockmendel added this to the 3.0 milestone Mar 14, 2023
@jbrockmendel
Copy link
Member Author

Error: None:None:GL01:pandas.tseries.offsets.Day.rule_code:Docstring text (summary) should start in the line immediately after the opening quotes (not in the same line, or leaving a blank line in between)
Error: None:None:GL02:pandas.tseries.offsets.Day.rule_code:Closing quotes should be placed in the line after the last text in the docstring (do not close the quotes in the same line as the text, or leave a blank line between the last text and the quotes)
Error: None:None:SS02:pandas.tseries.offsets.Day.rule_code:Summary does not start with a capital letter
Error: None:None:SS03:pandas.tseries.offsets.Day.rule_code:Summary does not end with a period
Error: None:None:SS06:pandas.tseries.offsets.Day.rule_code:Summary should fit in a single line

It looks like the linter is mistaking a non-docstring string for a docstring. @MarcoGorelli is this from the cython linter you implemented?

@MarcoGorelli
Copy link
Member

rule_code

nope, it's from scripts/validate_docstrings.py

@MarcoGorelli
Copy link
Member

is this ready for review? just asking because of the 'wip' in the title

@jbrockmendel
Copy link
Member Author

is this ready for review? just asking because of the 'wip' in the title

Ambiguous. It is a breaking change to be merged for 3.0. I made a PR on the theory that keeping it green for a year would be easier than rebasing it in a year. Ideally we can find some bits to break off as deprecations in the interim.

So reviews are welcome, but definitely not a priority.

@mroeschke
Copy link
Member

+1 to keep doing this

@jbrockmendel
Copy link
Member Author

Closing in favor of #55502

@jbrockmendel jbrockmendel deleted the daydst branch October 12, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets
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