bpo-39350: Fix regression introduced when fractions.gcd was removed #18309

mdickinson · 2020-02-02T10:06:37Z

This PR is a quick fix for the breakage introduced in #18021, which removed the _gcd function while the core code was still using it.

Possibly the behaviour of Fraction for non-integers should be different, but I think it makes sense to fix the breakage immediately and then discuss whether we want to make behavioural changes.

~~No news entry needed, because the broken code hasn't make it into any release or prerelease.~~ (EDIT: not true, unfortunately : @hroncok pointed out in comments below that the regression was released in 3.9.0a3)

https://bugs.python.org/issue39350

hroncok · 2020-02-02T10:19:58Z

No news entry needed, because the broken code hasn't make it into any release or prerelease.

The broken code made it to 3.9.0a3. That's how I have discovered the regression.

mdickinson · 2020-02-02T10:35:57Z

The broken code made it to 3.9.0a3.

Whoops; you're right. News entry needed, then!

codecov · 2020-02-02T12:13:12Z

Codecov Report

Merging #18309 into master will increase coverage by 1.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18309       +/-   ##
===========================================
+ Coverage   82.11%   83.19%    +1.07%     
===========================================
  Files        1955     1570      -385     
  Lines      588596   414126   -174470     
  Branches    44401    44402        +1     
===========================================
- Hits       483346   344525   -138821     
+ Misses      95605    59963    -35642     
+ Partials     9645     9638        -7

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
Lib/test/test_asyncio/test_base_events.py	`91.84% <0.00%> (-3.30%)`	⬇️
... and 435 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b94737a...2f0db20. Read the comment docs.

corona10 · 2020-02-02T15:28:29Z

Lib/fractions.py

@@ -13,6 +13,12 @@
 __all__ = ['Fraction']


+def _gcd(a, b):


Looks good, what about declared this function as the nested function?

In this case, we can skip the news :)

-def _gcd(a, b): - # Supports non-integers for backward compatibility. - while b: - a, b = b, a%b - return a - # Constants related to the hash implementation; hash(x) is based # on the reduction of x modulo the prime _PyHASH_MODULUS. _PyHASH_MODULUS = sys.hash_info.modulus @@ -98,6 +92,11 @@ class Fraction(numbers.Rational): """ self = super(Fraction, cls).__new__(cls) + def _gcd(a, b): + while b: + a, b = b, a%b + return a + if denominator is None: if type(numerator) is int: self._numerator = numerator

Looks good, what about declared this function as the nested function?

I'd prefer to revert to as close as possible to the previous code, which had _gcd as a module-level function.

I think we do need a news entry, because this is fixing a bug that was already made available in a release. If the bug had been introduced and then fixed without any intermediate release, then we could have skipped the news entry.

hroncok · 2020-02-04T09:44:51Z

FYI I'm now going to patch our Python we use to test Fedora packages with and will see if it actually fixes numpy (I am quite confident it will).

hroncok · 2020-02-04T14:47:44Z

it actually fixes numpy

hroncok

I like this as a fast one time fix of a regression with a test. Later, we can decide whether to redo the code not to use this _gcd function, but that is out of scope here.

mdickinson · 2020-02-04T18:20:11Z

@vstinner Okay to merge this?

vstinner · 2020-02-06T13:25:50Z

@vstinner Okay to merge this?

I don't see the need for _gcd(). I'm not sure that we have to keep the original type to compute the gcd. fractions.py code:

        if _normalize:
            g = math.gcd(numerator, denominator)
            if denominator < 0:
                g = -g
            numerator //= g
            denominator //= g

I don't think that it's a crazy requirement to support <your type> //= int: int is very common in Python.

I proposed PR #18375 which always use math.gcd().

mdickinson · 2020-02-06T14:44:39Z

@vstinner: Sure, but that's a behaviour change. I think we should fix the regression first (i.e., take the behaviour back to what it was, but with gcd removed), and then discuss possible behaviour changes.

vstinner · 2020-02-06T15:24:21Z

Lib/test/test_fractions.py

+
+        class myint(int):
+            def __mul__(self, other):
+                return myint(int(self)*int(other))


Suggested change

return myint(int(self)*int(other))

return myint(int(self) * int(other))

vstinner · 2020-02-06T15:24:33Z

Lib/fractions.py

+def _gcd(a, b):
+    # Supports non-integers for backward compatibility.
+    while b:
+        a, b = b, a%b


Suggested change

a, b = b, a%b

a, b = b, a % b

vstinner · 2020-02-06T15:27:33Z

Lib/fractions.py

@@ -13,6 +13,12 @@
 __all__ = ['Fraction']


+def _gcd(a, b):
+    # Supports non-integers for backward compatibility.


My main worry with this PR is this comment. I don't think that we should keep it back "backward compatibility". Either we remove _gcd() or we keep it on deliberate purpose, to return the same type than inputs (a and b arguments).

I understand that @mdickinson wants to return the same type, and I'm fine with that. But in this case, the comment should be something like:

For int sublcasses and other types, math.gcd(a, b) call the __index__() method on arguments and it always return the int type. This function is used to return the same type than input.

Maybe also add a comment in Fraction constructor to explain why we test type() == int: explain that we want to get g as the same type than numerator and denominator types. Or something like that ;-)

@vstinner This comment wasn't introduced in this PR; it's from the previously working code.

I understand that @mdickinson wants to return the same type

No. I simply want to undo the breakage that #18021 introduced, in the simplest way possible, and with a regression test. I'm open to discussing any sort of behaviour change after that, but the first priority should be to get back to a working state, and the simplest path is to get back to the working state that existed in 3.9.0a2.

Another way to address the breakage would be to revert #18021 in its entirety, and then re-do it more carefully.

The statistics module tries to keep input types for the output type. I think that it's good thing for generic modules like fractions and statistics. The documentation seems to say that it keeps the input type, but it's not very explicit: https://docs.python.org/dev/library/fractions.html

I'm fine with keeping Python 3.8 behavior (so use your PR). If someone wants to change it, it can be done later.

My first intent was only to remove the public deprecated fractions.gcd() function. I don't care about private functions like fractions._gcd().

vstinner · 2020-02-06T15:28:28Z

Lib/test/test_fractions.py

+                return myint(1)
+
+        myhalf = F(myint(1), myint(2))
+        self.assertEqual(myhalf, F(1, 2))


Can please add the following tests?

self.assertEqual(f.numerator, 1) self.assertEqual(f.denominator, 2) self.assertEqual(type(f.numerator), myint) self.assertEqual(type(f.denominator), myint)

vstinner · 2020-02-06T15:29:02Z

Lib/test/test_fractions.py

+            def denominator(self):
+                return myint(1)
+
+        myhalf = F(myint(1), myint(2))


I would prefer to see GCD being computed and used, so I propose:

Suggested change

myhalf = F(myint(1), myint(2))

myhalf = F(myint(1 * 3), myint(2 * 3))

vstinner · 2020-02-06T15:29:36Z

Lib/test/test_fractions.py

+        class myint(int):
+            def __mul__(self, other):
+                return myint(int(self)*int(other))
+


Can please add:

def __floordiv__(self, other): return type(self)(int(self) // int(other)) def __mod__(self, other): return type(self)(int(self) % int(other))

vstinner · 2020-02-06T15:30:41Z

Misc/NEWS.d/next/Library/2020-02-02-10-38-59.bpo-39350.8V69hi.rst

@@ -0,0 +1,3 @@
+Revert removal of ``fractions._gcd``. That removal caused breakage when
+using ``fractions.Fraction`` with integer-like values (for example NumPy
+integers).


You may reuse my NEWS entry to avoid mentioning the private _gcd() method, I don't think that it matters here:

Fix regression in :class:`fractions.Fraction` if the numerator and/or the denominator is an :class:`int` subclass.

bedevere-bot · 2020-02-06T15:31:28Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2020-02-06T15:33:14Z

@vstinner: Sure, but that's a behaviour change. I think we should fix the regression first (i.e., take the behaviour back to what it was, but with gcd removed), and then discuss possible behaviour changes.

I would prefer to only change the code once. Either remove _gcd() and use math.gcd() which returns an int for all input types (behavior change): PR #18375, or don't use math.gcd() for types others than int: this PR.

mdickinson · 2020-02-07T19:17:37Z

#18375 obviates the need for this fix

vstinner · 2020-02-07T22:43:26Z

I merged my PR #18375.

Fix regression introduced when fractions.gcd was removed

b8ae9b9

mdickinson added the skip news label Feb 2, 2020

mdickinson requested a review from vstinner February 2, 2020 10:06

the-knights-who-say-ni added the CLA signed label Feb 2, 2020

bedevere-bot added the awaiting core review label Feb 2, 2020

mdickinson removed the skip news label Feb 2, 2020

mdickinson added 2 commits February 2, 2020 10:39

Add news entry.

f634278

Fix indentation

2f0db20

corona10 reviewed Feb 2, 2020

View reviewed changes

corona10 approved these changes Feb 2, 2020

View reviewed changes

hroncok approved these changes Feb 4, 2020

View reviewed changes

vstinner mentioned this pull request Feb 6, 2020

bpo-39350: Fix fractions for int subclasses #18375

Merged

vstinner requested changes Feb 6, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Feb 6, 2020

mdickinson closed this Feb 7, 2020

mdickinson deleted the fix/gcd-regression branch February 7, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39350: Fix regression introduced when fractions.gcd was removed #18309

bpo-39350: Fix regression introduced when fractions.gcd was removed #18309

mdickinson commented Feb 2, 2020 •

edited

hroncok commented Feb 2, 2020

mdickinson commented Feb 2, 2020

codecov bot commented Feb 2, 2020

corona10 Feb 2, 2020

corona10 Feb 2, 2020

corona10 Feb 2, 2020 •

edited

mdickinson Feb 2, 2020

corona10 Feb 2, 2020

hroncok commented Feb 4, 2020 •

edited

hroncok commented Feb 4, 2020

hroncok left a comment

mdickinson commented Feb 4, 2020

vstinner commented Feb 6, 2020

mdickinson commented Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

mdickinson Feb 6, 2020 •

edited

vstinner Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

vstinner Feb 6, 2020

bedevere-bot commented Feb 6, 2020

vstinner commented Feb 6, 2020

mdickinson commented Feb 7, 2020

vstinner commented Feb 7, 2020

	return myint(int(self)*int(other))
	return myint(int(self) * int(other))

	myhalf = F(myint(1), myint(2))
	myhalf = F(myint(1 * 3), myint(2 * 3))

bpo-39350: Fix regression introduced when fractions.gcd was removed #18309

bpo-39350: Fix regression introduced when fractions.gcd was removed #18309

Conversation

mdickinson commented Feb 2, 2020 • edited

hroncok commented Feb 2, 2020

mdickinson commented Feb 2, 2020

codecov bot commented Feb 2, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 Feb 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hroncok commented Feb 4, 2020 • edited

hroncok commented Feb 4, 2020

hroncok left a comment

Choose a reason for hiding this comment

mdickinson commented Feb 4, 2020

vstinner commented Feb 6, 2020

mdickinson commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson Feb 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 6, 2020

vstinner commented Feb 6, 2020

mdickinson commented Feb 7, 2020

vstinner commented Feb 7, 2020

mdickinson commented Feb 2, 2020 •

edited

corona10 Feb 2, 2020 •

edited

hroncok commented Feb 4, 2020 •

edited

mdickinson Feb 6, 2020 •

edited