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

gh-91851: Trivial optimizations in Fraction #100791

Merged
merged 3 commits into from
Jan 6, 2023
Merged

gh-91851: Trivial optimizations in Fraction #100791

merged 3 commits into from
Jan 6, 2023

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Jan 6, 2023

@hauntsaninja hauntsaninja changed the title Trivial optimizations in Fraction gh-91851: Trivial optimizations in Fraction Jan 6, 2023
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks! Microbenchmarks confirm that this is about 2x as fast.

cc @JelleZijlstra

@mdickinson
Copy link
Member

mdickinson commented Jan 6, 2023

LGTM.

That's a pretty clunky implementation of round-ties-to-even in the ndigits is None case. Would something like the following be faster?

d = self._denominator
rounded, remainder = divmod(self._numerator + (d >> 1), d)
if not remainder and not (d & 1):  # tie case
    rounded &= -2
return rounded

EDIT: A bit more streamlined, and avoiding unnecessary not operations:

d = self._denominator
rounded, remainder = divmod(self._numerator + (d >> 1), d)
return rounded if remainder or d & 1 else rounded & -2

@mdickinson
Copy link
Member

Would something like the following be faster?

Turns out not, at least on my machine.

@mdickinson mdickinson merged commit 0e64026 into python:main Jan 6, 2023
@skirpichev skirpichev deleted the micro-opt-fractions-1 branch January 6, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants