classification
Title: Remove deprecated fractions.gcd()
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: hroncok, mark.dickinson, vstinner
Priority: normal Keywords: patch

Created on 2020-01-16 08:13 by vstinner, last changed 2020-02-07 22:44 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 18021 merged vstinner, 2020-01-16 08:17
PR 18309 closed mark.dickinson, 2020-02-02 10:06
PR 18375 merged vstinner, 2020-02-06 12:42
Messages (12)
msg360095 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-16 08:13
bpo-22486 added math.gcd() and deprecated fractions.gcd() in Python 3.5: commit 48e47aaa28d6dfdae128142ffcbc4b0642422e90.

The function was deprecated during 4 cycles (3.5, 3.6, 3.7, 3.8): I propose attached PR to remove it.
msg360111 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-16 10:02
New changeset 4691a2f2a2b8174a6c958ce6976ed5f3354c9504 by Victor Stinner in branch 'master':
bpo-39350: Remove deprecated fractions.gcd() (GH-18021)
https://github.com/python/cpython/commit/4691a2f2a2b8174a6c958ce6976ed5f3354c9504
msg361103 - (view) Author: Miro Hrončok (hroncok) * Date: 2020-01-31 12:52
I believe there is a regression here.

numpy fails to build with Python 3.9.0a3.

________________________ TestMatmul.test_matmul_object _________________________
self = <numpy.core.tests.test_multiarray.TestMatmul object at 0x7ff9bc008ca0>
    def test_matmul_object(self):
        import fractions
    
        f = np.vectorize(fractions.Fraction)
        def random_ints():
            return np.random.randint(1, 1000, size=(10, 3, 3))
>       M1 = f(random_ints(), random_ints())
f          = <numpy.vectorize object at 0x7ff9bc6751f0>
fractions  = <module 'fractions' from '/usr/lib64/python3.9/fractions.py'>
random_ints = <function TestMatmul.test_matmul_object.<locals>.random_ints at 0x7ff9bcc2d700>
self       = <numpy.core.tests.test_multiarray.TestMatmul object at 0x7ff9bc008ca0>
numpy/core/tests/test_multiarray.py:6259: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
numpy/lib/function_base.py:2091: in __call__
    return self._vectorize_call(func=func, args=vargs)
numpy/lib/function_base.py:2161: in _vectorize_call
    ufunc, otypes = self._get_ufunc_and_otypes(func=func, args=args)
numpy/lib/function_base.py:2121: in _get_ufunc_and_otypes
    outputs = func(*inputs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cls = <class 'fractions.Fraction'>, numerator = 483, denominator = 809
    def __new__(cls, numerator=0, denominator=None, *, _normalize=True):
        """Constructs a Rational.
    
        Takes a string like '3/2' or '1.5', another Rational instance, a
        numerator/denominator pair, or a float.
    
        Examples
        --------
    
        >>> Fraction(10, -8)
        Fraction(-5, 4)
        >>> Fraction(Fraction(1, 7), 5)
        Fraction(1, 35)
        >>> Fraction(Fraction(1, 7), Fraction(2, 3))
        Fraction(3, 14)
        >>> Fraction('314')
        Fraction(314, 1)
        >>> Fraction('-35/4')
        Fraction(-35, 4)
        >>> Fraction('3.1415') # conversion from numeric string
        Fraction(6283, 2000)
        >>> Fraction('-47e-2') # string may include a decimal exponent
        Fraction(-47, 100)
        >>> Fraction(1.47)  # direct construction from float (exact conversion)
        Fraction(6620291452234629, 4503599627370496)
        >>> Fraction(2.25)
        Fraction(9, 4)
        >>> Fraction(Decimal('1.47'))
        Fraction(147, 100)
    
        """
        self = super(Fraction, cls).__new__(cls)
    
        if denominator is None:
            if type(numerator) is int:
                self._numerator = numerator
                self._denominator = 1
                return self
    
            elif isinstance(numerator, numbers.Rational):
                self._numerator = numerator.numerator
                self._denominator = numerator.denominator
                return self
    
            elif isinstance(numerator, (float, Decimal)):
                # Exact conversion
                self._numerator, self._denominator = numerator.as_integer_ratio()
                return self
    
            elif isinstance(numerator, str):
                # Handle construction from strings.
                m = _RATIONAL_FORMAT.match(numerator)
                if m is None:
                    raise ValueError('Invalid literal for Fraction: %r' %
                                     numerator)
                numerator = int(m.group('num') or '0')
                denom = m.group('denom')
                if denom:
                    denominator = int(denom)
                else:
                    denominator = 1
                    decimal = m.group('decimal')
                    if decimal:
                        scale = 10**len(decimal)
                        numerator = numerator * scale + int(decimal)
                        denominator *= scale
                    exp = m.group('exp')
                    if exp:
                        exp = int(exp)
                        if exp >= 0:
                            numerator *= 10**exp
                        else:
                            denominator *= 10**-exp
                if m.group('sign') == '-':
                    numerator = -numerator
    
            else:
                raise TypeError("argument should be a string "
                                "or a Rational instance")
    
        elif type(numerator) is int is type(denominator):
            pass # *very* normal case
    
        elif (isinstance(numerator, numbers.Rational) and
            isinstance(denominator, numbers.Rational)):
            numerator, denominator = (
                numerator.numerator * denominator.denominator,
                denominator.numerator * numerator.denominator
                )
        else:
            raise TypeError("both arguments should be "
                            "Rational instances")
    
        if denominator == 0:
            raise ZeroDivisionError('Fraction(%s, 0)' % numerator)
        if _normalize:
            if type(numerator) is int is type(denominator):
                # *very* normal case
                g = math.gcd(numerator, denominator)
                if denominator < 0:
                    g = -g
            else:
>               g = _gcd(numerator, denominator)
E               NameError: name '_gcd' is not defined
__class__  = <class 'fractions.Fraction'>
_normalize = True
cls        = <class 'fractions.Fraction'>
denominator = 809
numerator  = 483
self       = <[AttributeError("_numerator") raised in repr()] Fraction object at 0x7ff9bc6753a0>
/usr/lib64/python3.9/fractions.py:164: NameError


This removed _gcd, but it is still called in:

https://github.com/python/cpython/blob/58a4054760bffbb20aff90290dd0f3554f7bea42/Lib/fractions.py#L164
msg361104 - (view) Author: Miro Hrončok (hroncok) * Date: 2020-01-31 13:09
Reproducer:

class myint(int):
    def __mul__(self, other):
        return type(self)(int(self)*int(other))
    @property
    def numerator(self):
        return type(self)(super().numerator)
    @property
    def denominator(self):
        return type(self)(super().denominator)


Before:

>>> fractions.Fraction(myint(1), myint(2))
Fraction(1, 2)


After:

>>> fractions.Fraction(myint(1), myint(2))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.9/fractions.py", line 164, in __new__
    g = _gcd(numerator, denominator)
NameError: name '_gcd' is not defined
msg361111 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-01-31 14:44
The relevant piece of Python code from fractions.py looks like this:

    if type(numerator) is int is type(denominator):
        # *very* normal case
        g = math.gcd(numerator, denominator)
        if denominator < 0:
            g = -g
    else:
        g = _gcd(numerator, denominator)

I wonder whether we should simply drop the type check: math.gcd should accept anything that supports __index__ (and that includes NumPy integer types and anything that implements numbers.Integral).

OTOH, math.gcd will return a plain int, and if we have some subclass myint of int, we might want all the arithmetic to stay in the domain of myints (though I'm not really sure _why_ we'd want that). In that case we'll need to restore the pure Python _gcd implementation.
msg361126 - (view) Author: Miro Hrončok (hroncok) * Date: 2020-01-31 19:30
Naively implementing this code, I'd use isinstance(numerator, int) over type(numerator) is int. Is that strict type check really needed?

I could not find anywhere whether numerator and denominator of numbers.Rational need to be numbers.Integral. I would expect so, however this is not written in the documentation of numbers.Rational. If that's the case, I think checking it for being numbers.Integral and failing hard if not is a reasonable thing to do.
msg361225 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-02-02 10:08
Apologies, I think I moved the discussion off-track. The first order of business should be to fix the regression. We can discuss behaviour changes after that.

I've opened GH-18309 as a quick fix for the regression.
msg361481 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-06 13:42
I tried but failed to write a test to mimick numpy.int64 type. I tried to build a type which implements numbers.Rational but don't inherit from int, but there are way too many methods that I have to implement :-(

Morever, installing numpy on a Python 3.9 virtual environment is quite tricky. Lhe latest Cython release (0.29.14) isn't compatible with Python 3.9.

Miro gave me a command to install Cython on Python 3.9:

python -m pip install https://github.com/cython/cython/archive/master.tar.gz --install-option="--no-cython-compile"

But then "pip install numpy" tries to reinstall Cython which fails :-/
msg361483 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-06 14:19
I managed to test my PR with numpy:

$ env/bin/python 
>>> import fractions
>>> import numpy
>>> f=fractions.Fraction(numpy.int64(1*3), numpy.int64(2*3))
>>> f
Fraction(1, 2)
>>> type(f.numerator)
<class 'numpy.int64'>
>>> type(f.denominator)
<class 'numpy.int64'>

So it works as expected: numerator and denominator have the expected type, and there is no exception ;-)

I used the following commands to get numpy in a Python 3.9 virtual environment:

./python -m venv env
env/bin/python -m pip install https://github.com/cython/cython/archive/master.tar.gz --install-option="--no-cython-compile"
curl -O https://files.pythonhosted.org/packages/40/de/0ea5092b8bfd2e3aa6fdbb2e499a9f9adf810992884d414defc1573dca3f/numpy-1.18.1.zip
unzip -d . numpy-1.18.1.zip
cd numpy-1.18.1/
../env/bin/python setup.py install
msg361485 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-06 14:34
FYI the full numpy test suite pass with PR 18375:

(env) vstinner@apu$ python runtests.py  -v
(...)
======================= 9879 passed, 443 skipped, 180 deselected, 17 xfailed, 3 xpassed in 305.17s (0:05:05) =======================
msg361612 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-07 22:42
New changeset dc7a50d73a3d16918529669ff7b8783c08cff090 by Victor Stinner in branch 'master':
bpo-39350: Fix fractions for int subclasses (GH-18375)
https://github.com/python/cpython/commit/dc7a50d73a3d16918529669ff7b8783c08cff090
msg361613 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-07 22:44
Thanks Miro for the bug report and thanks Mark for the great review and discussion! It should now be fixed. I tested manually that my change fix numpy test suite.
History
Date User Action Args
2020-02-07 22:44:10vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg361613

stage: patch review -> resolved
2020-02-07 22:42:55vstinnersetmessages: + msg361612
2020-02-06 14:34:29vstinnersetmessages: + msg361485
2020-02-06 14:19:57vstinnersetmessages: + msg361483
2020-02-06 13:42:26vstinnersetresolution: fixed -> (no value)
messages: + msg361481
2020-02-06 12:42:10vstinnersetpull_requests: + pull_request17751
2020-02-02 10:08:55mark.dickinsonsetmessages: + msg361225
2020-02-02 10:06:39mark.dickinsonsetstage: resolved -> patch review
pull_requests: + pull_request17685
2020-01-31 19:30:00hroncoksetmessages: + msg361126
2020-01-31 14:44:47mark.dickinsonsetstatus: closed -> open

messages: + msg361111
2020-01-31 14:19:55mark.dickinsonsetnosy: + mark.dickinson
2020-01-31 13:09:43hroncoksetmessages: + msg361104
2020-01-31 12:52:22hroncoksetnosy: + hroncok
messages: + msg361103
2020-01-16 10:03:19vstinnersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-01-16 10:02:58vstinnersetmessages: + msg360111
2020-01-16 08:17:23vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request17416
2020-01-16 08:13:44vstinnercreate