gh-101773: Optimize creation of Fractions in private methods

[skirpichev]

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

• Issue: Optimize creation of unnormalized Fraction's in private methods (arithmetics, etc) #101773

[skirpichev]

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.

[mdickinson]

@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]

On Sat, Feb 11, 2023 at 04:53:50AM -0800, Mark Dickinson wrote: I think you can and should assume that all core developers are very familiar with PEP 8. :-)

Here I do care more about users knowledge. The single leading underscore as a stigmate of private (internaly used) entity is more or less common convention across the python ecosystem. Sometimes it's enforced (e.g. help() hides such methods, this stuff is not included in star-imports and so on). I wonder if we shouldn't do same with _-prefixed kwargs (at least in help()).

That's a rather dogmatic approach. On CPython, we should prefer a more pragmatic approach: if we have reasonable grounds to believe that a change might break user code then we should take reasonable steps to avoid that breakage. It's not friendly to knowingly break existing code and then tell people "well, it was your fault for using something undocumented".

But the Python is a dynamical language and with great introspection capabilities... In this way we can end with supporting everything as part of the API, because people actually *can* use everything.

[skirpichev]

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

[mdickinson]

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.

[bedevere-bot]

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.

[skirpichev]

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]

Thanks for making the requested changes!

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

[eendebakpt]

@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.

[mdickinson]

LGTM. I tweaked the docstring wording on _from_coprime_ints slightly.

I'll merge when CI completes.

[skirpichev]

Thanks to all reviewers.