Skip to content

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

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.

Sign up for GitHub

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

Closed
mdickinson wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8ae9b9
Fix regression introduced when fractions.gcd was removed
mdickinson Feb 2, 2020
f634278
Add news entry.
mdickinson Feb 2, 2020
2f0db20
Fix indentation
mdickinson Feb 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Lib/fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
__all__ = ['Fraction']


def _gcd(a, b):
Copy link
Copy Markdown
Member

@corona10 corona10 Feb 2, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@corona10 corona10 Feb 2, 2020

Choose a reason for hiding this comment

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

In this case, we can skip the news :)

Copy link
Copy Markdown
Member

@corona10 corona10 Feb 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

-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

Copy link
Copy Markdown
Member Author

@mdickinson mdickinson Feb 2, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@corona10 corona10 Feb 2, 2020

Choose a reason for hiding this comment

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

Got it :)

# Supports non-integers for backward compatibility.
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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 ;-)

Copy link
Copy Markdown
Member Author

@mdickinson mdickinson Feb 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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().

while b:
a, b = b, a%b
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

Suggested change
a, b = b, a%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
Expand Down
18 changes: 18 additions & 0 deletions Lib/test/test_fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,24 @@ def test_slots(self):
r = F(13, 7)
self.assertRaises(AttributeError, setattr, r, 'a', 10)

def test_gcd_compatibility_branch(self):
# Regression test for bugs.python.org/issue39350

class myint(int):
def __mul__(self, other):
return myint(int(self)*int(other))
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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


Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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))

@property
def numerator(self):
return self

@property
def denominator(self):
return myint(1)

myhalf = F(myint(1), myint(2))
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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))

self.assertEqual(myhalf, F(1, 2))
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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)



if __name__ == '__main__':
unittest.main()
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2020-02-02-10-38-59.bpo-39350.8V69hi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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).
Copy link
Copy Markdown
Member

@vstinner vstinner Feb 6, 2020

Choose a reason for hiding this comment

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

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.