Skip to content

BUG: Tick division is incorrect #57508

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

Closed
wants to merge 3 commits into from

Conversation

lithomas1
Copy link
Member

# self._value < other, we would get 0 as a result,
# which is not correct.
# We need to try going to a higher resolution to fix this
curr_value = self._value
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not the happiest with this code, since there's a possibility we might overflow (if other is very big), and there could be the possibility that a Tick is not representable in nanoseconds.

Is this a real possibility?
(not too familiar with tick code, so I can't tell mysel)

Copy link
Member

Choose a reason for hiding this comment

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

this introduces (untested) changes in Timedelta division in addition to Tick. I'm skeptical that this is the right place for a change, but if it is, it should be tested.

@lithomas1 lithomas1 added the Frequency DateOffsets label Feb 19, 2024
@lithomas1 lithomas1 added this to the 2.2.1 milestone Feb 20, 2024
@jbrockmendel
Copy link
Member

did you look at Tick._next_higher_resolution? it is what we use in Tick.__mul__ when multiplying by a float, and i suspect that logic can be adapted pretty cleanly

@lithomas1 lithomas1 modified the milestones: 2.2.1, 2.2.2 Feb 23, 2024
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 Mar 25, 2024
@lithomas1 lithomas1 removed this from the 2.2.2 milestone Apr 9, 2024
@mroeschke
Copy link
Member

Looks like this PR has gone stale and needs a rebase. Closing to clear the queue but feel free to reopen when you have time to circle back

@mroeschke mroeschke closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: pandas.tseries.offsets.Second() division
3 participants