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-101773: Optimize creation of Fractions in private methods #101780

Merged
merged 13 commits into from
Feb 27, 2023

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Feb 10, 2023

  • removed private _normalize argument of the Fraction constructor
  • added _from_coprime_ints() helper to make fractions from a pair of coprime ints

Lib/fractions.py Outdated Show resolved Hide resolved
@skirpichev
Copy link
Member Author

Probably, from_float/decimal methods should be adapted to use this. Decimal.as_integer_ratio is in lowest terms. I guess, that same holds for the float method, but that's undocumented, unfortunately.

Lib/fractions.py Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

@skirpichev

I guess, that same holds for the float method, but that's undocumented, unfortunately

It does indeed. It would be reasonable to document it, I think.

@skirpichev
Copy link
Member Author

skirpichev commented Feb 11, 2023 via email

@mdickinson mdickinson requested review from mdickinson and eendebakpt and removed request for eendebakpt February 12, 2023 11:02
@merwok merwok changed the title gh-101773: Optimize creation of Fraction's in private methods gh-101773: Optimize creation of Fractions in private methods Feb 19, 2023
@skirpichev
Copy link
Member Author

@mdickinson, _normalize part was reverted. Does it make sense now?

@mdickinson
Copy link
Member

mdickinson commented Feb 26, 2023

In this way we can end with supporting everything as part of the API.

Indeed. :-) As with most aspects of software development, careful judgement is required, evaluating the context and balancing the trade offs within that context. Few things are black and white. The fact that _normalize was intended to be private, and has a leading underscore, certainly means that removing it is easier than if it were public, but it's still something that shouldn't be done without thinking it through first. Hyrum's law is a real thing.

@skirpichev

Does it make sense now?

Hmm, not really. :-) I don't want to leave a _normalize flag that's unused and untested, indefinitely. I think we have three options:

  1. Leave this alone.
  2. Introduce _from_pair (possibly after renaming - see below) and immediately remove the _normalize flag
  3. Introduce _from_pair (ditto) and deprecate the _normalize flag.

Of those options, option 3 just seems like too much code churn for too little benefit.

There was a recent discuss.python.org thread in which the meaning of _normalize was misinterpreted, which pushes me a bit further towards option 2. Arguably, _normalize is too discoverable right now (since it shows up in IDE completions and tooltips), and there's a temptation to guess what it means. _from_pair would also be somewhat discoverable, but at least we get to add a docstring that clearly indicates that it's private, and the expected constraints for people who decide to use it anyway.

Screenshot 2023-02-26 at 12 52 41

Lib/fractions.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mdickinson mdickinson self-assigned this Feb 26, 2023
@skirpichev
Copy link
Member Author

skirpichev commented Feb 27, 2023

Arguably, _normalize is too discoverable right now (since it shows up in IDE completions and tooltips)

That is a more generic problem: we have PEP8's convention where _single_leading_underscore is weak “internal use” indicator. On another hand, these arguments are available for introspection and shown in the help() output, IDE tooltips and so on.

Probably, all above use the signature() function. Maybe it does make sense to add a private=False option for this function to hide private kwargs per default?

There was a recent discuss.python.org thread in which the meaning of _normalize was misinterpreted

(Probably it was about this thread.) Indeed, there was an impression that all methods do support unnormalized fractions.

offtopic On another hand 1. there is no doubts that the argument was private 2. most methods actually do support this (arithmetics) and 3. much more can if we do a cheap normalization (ensure that denominator is positive)

So, I think the proposal might have some sense and it's possible to implement it with a little overhead for canonicalized fractions. E.g. we can mark unnormalized fractions with a sticky flag (or flags) and do special steps to deal with this case in some methods. Not sure it worth to have this in the stdlib: to really support unnormalized fractions almost every method should be rewritten (e.g. arithmetic methods do some normalization of the output, which we probably want to avoid for unnormalized fractions).

PS:
I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

@eendebakpt
Copy link
Contributor

@skirpichev @mdickinson The changes look good to me. With the removal of the (semi-private) argument _normalize, we could update the whatsnew entry with a note stating that it is removed.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM. I tweaked the docstring wording on _from_coprime_ints slightly.

I'll merge when CI completes.

@mdickinson mdickinson merged commit 4f3786b into python:main Feb 27, 2023
@skirpichev skirpichev deleted the opt-fractions-new branch February 28, 2023 02:23
@skirpichev
Copy link
Member Author

Thanks to all reviewers.

carljm added a commit to carljm/cpython that referenced this pull request Feb 28, 2023
* main: (67 commits)
  pythongh-99108: Add missing md5/sha1 defines to Modules/Setup (python#102308)
  pythongh-100227: Move _str_replace_inf to PyInterpreterState (pythongh-102333)
  pythongh-100227: Move the dtoa State to PyInterpreterState (pythongh-102331)
  pythonGH-102305: Expand some macros in generated_cases.c.h (python#102309)
  Migrate to new PSF mailgun account (python#102284)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Python/) (python#102193)
  pythonGH-90744: Fix erroneous doc links in the sys module (python#101319)
  pythongh-87092: Make jump target label equal to the offset of the target in the instructions sequence (python#102093)
  pythongh-101101: Unstable C API tier (PEP 689) (pythonGH-101102)
  IDLE: Simplify DynOptionsMenu __init__code (python#101371)
  pythongh-101561: Add typing.override decorator (python#101564)
  pythongh-101825: Clarify that as_integer_ratio() output is always normalized (python#101843)
  pythongh-101773: Optimize creation of Fractions in private methods (python#101780)
  pythongh-102251: Updates to test_imp Toward Fixing Some Refleaks (pythongh-102254)
  pythongh-102296 Document that inspect.Parameter kinds support ordering (pythonGH-102297)
  pythongh-102250: Fix double-decref in COMPARE_AND_BRANCH error case (pythonGH-102287)
  pythongh-101100: Fix sphinx warnings in `types` module (python#102274)
  pythongh-91038: Change default argument value to `False` instead of `0` (python#31621)
  pythongh-101765: unicodeobject: use Py_XDECREF correctly (python#102283)
  [doc] Improve grammar/fix missing word (pythonGH-102060)
  ...
scoder added a commit to scoder/quicktions that referenced this pull request Mar 19, 2023
…numerator/denominator pair is already normalised.

Follows python/cpython#101780
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Mar 21, 2023
https://build.opensuse.org/request/show/1073017
by user dgarcia + dimstar_suse
- Enable python 3.11 build again, now is supported
- Update to 1.14
  - Implement __format__ for Fraction, following python/cpython#100161
  - Implement Fraction.is_integer(), following python/cpython#100488
  - Fraction.limit_denominator() is faster, following
    python/cpython#93730
  - Internal creation of result Fractions is about 10% faster if the
    calculated numerator/denominator pair is already normalised,
    following python/cpython#101780
  - Built using Cython 3.0.0b1.
- 1.13
  - Parsing very long numbers from a fraction string was very slow,
    even slower than fractions.Fraction. The parser is now faster in
    all cases (and still much faster for shorter numbers).
  - Fraction did not implement __int__.
    https://bugs.python.org/issue44547
- 1.12
  - Faster and more spa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants