-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
WIP/API: Make Day not a Tick #51874
Conversation
On first glance, I am a little surprised how much 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. |
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. |
We could probably restore+deprecate Day multiplication and division, maybe also Timedelta(Day) and Day.nanos |
Some of this will be unnecessary with #51896 |
+1 on the idea, 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. |
There was a problem hiding this 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
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. |
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. |
It looks like the linter is mistaking a non-docstring string for a docstring. @MarcoGorelli is this from the cython linter you implemented? |
nope, it's from |
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. |
+1 to keep doing this |
Closing in favor of #55502 |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.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.