From 599c55b46e8d751586e3245ea51efcfe66bfaac7 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Fri, 3 Jun 2022 11:11:38 -1000 Subject: [PATCH 1/7] gh-93476: Fraction.limit_denominator speed increases Create only one new Fraction object during `limit_denominator()` instead of the four previously made. Fixes #93476 I believe that each of the calls to `Fraction()` can be called with `_normalize=False` but I have not included that in this PR to limit the scope of changes to the minimum to verify no change in implementation. --- Lib/fractions.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index f9ac882ec002fa..dbc1d71fa9cdf2 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -234,10 +234,11 @@ def limit_denominator(self, max_denominator=1000000): if max_denominator < 1: raise ValueError("max_denominator should be at least 1") if self._denominator <= max_denominator: - return Fraction(self) + return Fraction(self._numerator, self._denominator) p0, q0, p1, q1 = 0, 1, 1, 0 n, d = self._numerator, self._denominator + n_original, d_original = n, d while True: a = n//d q2 = q0+a*q1 @@ -247,12 +248,23 @@ def limit_denominator(self, max_denominator=1000000): n, d = d, n-a*d k = (max_denominator-q0)//q1 - bound1 = Fraction(p0+k*p1, q0+k*q1) - bound2 = Fraction(p1, q1) - if abs(bound2 - self) <= abs(bound1-self): - return bound2 + bound1_n = p0 + k*p1 + bound1_d = q0 + k*q1 + bound2_n = p1 + bound2_d = q1 + bound1_minus_self_n = abs((bound1_n * d_original) + - (n_original * bound1_d)) + bound1_minus_self_d = d_original * bound1_d + bound2_minus_self_n = abs((bound2_n * d_original) + - (n_original * bound2_d)) + bound2_minus_self_d = d_original * bound2_d + + difference = ((bound1_minus_self_n * bound2_minus_self_d) + - (bound2_minus_self_n * bound1_minus_self_d)) + if difference > 0: + return Fraction(bound2_n, bound2_d) else: - return bound1 + return Fraction(bound1_n, bound1_d) @property def numerator(a): From 70ca7ad26b7a16b885682b628a7ef24122535b41 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Fri, 3 Jun 2022 11:18:05 -1000 Subject: [PATCH 2/7] add NEWS.d item --- .../next/Library/2022-06-03-11-17-49.gh-issue-93476.vvjcGL.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-06-03-11-17-49.gh-issue-93476.vvjcGL.rst diff --git a/Misc/NEWS.d/next/Library/2022-06-03-11-17-49.gh-issue-93476.vvjcGL.rst b/Misc/NEWS.d/next/Library/2022-06-03-11-17-49.gh-issue-93476.vvjcGL.rst new file mode 100644 index 00000000000000..09298d31f47cdd --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-06-03-11-17-49.gh-issue-93476.vvjcGL.rst @@ -0,0 +1,2 @@ +:meth:`fractions.Fraction.limit_denominator()` performance enhancements. +Patch by Michael Scott Asato Cuthbert. From a1a4fc863d58dbcd25f0b2aab4de162ce2967064 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Fri, 3 Jun 2022 14:03:19 -1000 Subject: [PATCH 3/7] Fix boundary case. --- Lib/fractions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index dbc1d71fa9cdf2..c2859c8091faeb 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -234,7 +234,7 @@ def limit_denominator(self, max_denominator=1000000): if max_denominator < 1: raise ValueError("max_denominator should be at least 1") if self._denominator <= max_denominator: - return Fraction(self._numerator, self._denominator) + return Fraction(self._numerator, self._denominator, _normalize=False) p0, q0, p1, q1 = 0, 1, 1, 0 n, d = self._numerator, self._denominator @@ -261,7 +261,7 @@ def limit_denominator(self, max_denominator=1000000): difference = ((bound1_minus_self_n * bound2_minus_self_d) - (bound2_minus_self_n * bound1_minus_self_d)) - if difference > 0: + if difference >= 0: return Fraction(bound2_n, bound2_d) else: return Fraction(bound1_n, bound1_d) From 3c06ded3ee1f152ee7e9467154efc5aa87c34bf2 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Sun, 5 Jun 2022 00:12:45 -1000 Subject: [PATCH 4/7] Review comments * Set variable naming according to existing cpython standards * confirm GCD of returned Fraction, and skip normalization * Respond and thanks to @mdickinson --- Lib/fractions.py | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index c2859c8091faeb..c2ee4d53e534ea 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -90,6 +90,13 @@ def __new__(cls, numerator=0, denominator=None, *, _normalize=True): Fraction(147, 100) """ + # private _normalize=False should only be set if the Fraction is + # already asserted to be normalized. + # (see discussion: at https://github.com/python/cpython/pull/93477) + # if a non-normalized Fraction is passed in with _normalize=False + # then API calls may give inconsistent results on equivalent + # Fraction objects. + self = super(Fraction, cls).__new__(cls) if denominator is None: @@ -252,19 +259,19 @@ def limit_denominator(self, max_denominator=1000000): bound1_d = q0 + k*q1 bound2_n = p1 bound2_d = q1 - bound1_minus_self_n = abs((bound1_n * d_original) - - (n_original * bound1_d)) - bound1_minus_self_d = d_original * bound1_d - bound2_minus_self_n = abs((bound2_n * d_original) - - (n_original * bound2_d)) - bound2_minus_self_d = d_original * bound2_d - - difference = ((bound1_minus_self_n * bound2_minus_self_d) - - (bound2_minus_self_n * bound1_minus_self_d)) - if difference >= 0: - return Fraction(bound2_n, bound2_d) + + # diff1_n = numerator of (bound1 minus self) as a Fraction; + # etc. for diff1_d, diff2_n, diff2_d + diff1_n = abs(bound1_n*d_original - n_original*bound1_d) + diff1_d = d_original * bound1_d + diff2_n = abs(bound2_n*d_original - n_original*bound2_d) + diff2_d = d_original * bound2_d + + if diff1_n * diff2_d >= diff2_n * diff1_d: + # bound2 is closer (or equal) to original as bound 1 + return Fraction(bound2_n, bound2_d, _normalize=False) else: - return Fraction(bound1_n, bound1_d) + return Fraction(bound1_n, bound1_d, _normalize=False) @property def numerator(a): From e5ab32ec49c8d329e1bdfe90eec2bb77a67b9e77 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Sun, 5 Jun 2022 00:14:43 -1000 Subject: [PATCH 5/7] trailing space --- Lib/fractions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index c2ee4d53e534ea..02cbd1f29f9c3b 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -260,7 +260,7 @@ def limit_denominator(self, max_denominator=1000000): bound2_n = p1 bound2_d = q1 - # diff1_n = numerator of (bound1 minus self) as a Fraction; + # diff1_n = numerator of (bound1 minus self) as a Fraction; # etc. for diff1_d, diff2_n, diff2_d diff1_n = abs(bound1_n*d_original - n_original*bound1_d) diff1_d = d_original * bound1_d From 40cbe78d140c3f3e52b239cc8fc0671715ce4490 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Sun, 5 Jun 2022 02:02:26 -1000 Subject: [PATCH 6/7] return Fraction(self) for limit case --- Lib/fractions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index 02cbd1f29f9c3b..ea7b73ad8b9212 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -241,7 +241,7 @@ def limit_denominator(self, max_denominator=1000000): if max_denominator < 1: raise ValueError("max_denominator should be at least 1") if self._denominator <= max_denominator: - return Fraction(self._numerator, self._denominator, _normalize=False) + return Fraction(self) p0, q0, p1, q1 = 0, 1, 1, 0 n, d = self._numerator, self._denominator From f5daa21f4b96195771e01d420b16e7b5abf1a439 Mon Sep 17 00:00:00 2001 From: Michael Scott Asato Cuthbert Date: Sun, 5 Jun 2022 02:09:03 -1000 Subject: [PATCH 7/7] drop comment for now. can be separate PR --- Lib/fractions.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Lib/fractions.py b/Lib/fractions.py index ea7b73ad8b9212..ea7e5b8f18e296 100644 --- a/Lib/fractions.py +++ b/Lib/fractions.py @@ -90,13 +90,6 @@ def __new__(cls, numerator=0, denominator=None, *, _normalize=True): Fraction(147, 100) """ - # private _normalize=False should only be set if the Fraction is - # already asserted to be normalized. - # (see discussion: at https://github.com/python/cpython/pull/93477) - # if a non-normalized Fraction is passed in with _normalize=False - # then API calls may give inconsistent results on equivalent - # Fraction objects. - self = super(Fraction, cls).__new__(cls) if denominator is None: