This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Fix missing test coverage for fractions.Fraction.__new__
Type: Stage: resolved
Components: Library (Lib), Tests Versions: Python 3.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: Sergey.Kirpichev, gphemsley, mark.dickinson, rhettinger
Priority: normal Keywords:

Created on 2017-12-31 14:53 by gphemsley, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (13)
msg309288 - (view) Author: Gordon P. Hemsley (gphemsley) * Date: 2017-12-31 14:53
I noticed that there was a single line of Lib/fractions.py that did not have test coverage: the normalize step for fractions with non-integer numerators and/or denominators.

I initially was going to implement a test for that line, but upon further reflection, I cannot envision a scenario where that would be possible. The code already requires its initial parameters to be numbers.Rational and then normalizes their numerators and denominators to be integers.

So, instead, I propose to remove this check, first introduced in issue22486, and to roll the fractions._gcd() function up into the deprecated fractions.gcd().
msg309290 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-12-31 16:08
> [...] I cannot envision a scenario where that would be possible [...]

I don't think it can be ruled out. If I'm reading the code right, it's preceded by this branch of the initial if/elif chain:

        elif (isinstance(numerator, numbers.Rational) and
            isinstance(denominator, numbers.Rational)):
            numerator, denominator = (
                numerator.numerator * denominator.denominator,
                denominator.numerator * numerator.denominator
                )

I don't think there's any guarantee that if `numerator` is an instance of `numbers.Rational`, then `numerator.numerator` and `numerator.denominator` have exact type `int`.  (Note that an instance of `numbers.Rational` is not necessarily an instance of `fractions.Fraction`.)
msg309297 - (view) Author: Gordon P. Hemsley (gphemsley) * Date: 2017-12-31 18:53
Indeed, that is the code fragment I was referring to.

Mathematically speaking, a rational number is one that can be expressed as a fraction of two integers, so in that regard the numerator and the denominator should both be integers.

But let's assume for argument's sake that a type comes through where the numerator and the denominator are fractions: 1/2 and 2/3, respectively. This code would normalize them by cross-multiplying:

numerator = 1 * 3 = 3
denominator = 2 * 3 = 6

Now they are both integers.

In what scenario would the numerator and denominator be numbers.Rational but not an integer or a fraction? If someone could even come up with one, would it be worthwhile to allow as a fraction?

And on the flip side, if math.gcd() only accepts integers and not, at least, numbers.Integral, wouldn't that be a bug?
msg309299 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-12-31 19:33
> In what scenario would the numerator and denominator be numbers.Rational but not an integer or a fraction

But that's not the issue here. The issue here is having an instance of `numbers.Rational` whose numerator and denominator are not specifically of concrete type `int`: the test that (IIUC) you're proposing to remove is:

     if type(numerator) is int is type(denominator):
         [...]

Certainly I'd expect the numerator and denominator to be instances of `numbers.Integral`, but that's more general than being of exact type `int`.

The `numbers.Integral` and `numbers.Rational` ABCs are deliberately not tied to particular concrete classes. It would be perfectly possible (and reasonable) for someone to have a `MyFraction` class that behaves like a `Fraction`, and whose numerator and denominator are instances of some other concrete class `MyInteger` that behaves like an integer.

The branch in the code is necessary to support that situation, so we can't simply remove it.
msg309304 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-12-31 20:23
Here's a concrete example, using gmpy2 [1], that exercises the code path you're proposing to remove. I had to cheat a bit, since the gmpy2 types *don't* (currently) buy in to the numbers ABC tower (though there's no good reason that they couldn't do so), so I registered them manually.

>>> import fractions, numbers
>>> import gmpy2
>>> x = gmpy2.mpq(2, 3)
>>> numbers.Rational.register(type(x))
<class 'mpq'>
>>> numbers.Integral.register(type(x.denominator))
<class 'mpz'>
>>> fractions.Fraction(x, x)  # code path exercised here ...
Fraction(1, 1)

Note that the numerator and the denominator of the resulting `Fraction` object are still of type `mpz`.

It _would_ be possible to remove the check and always use `math.gcd`, but it would result in a behaviour change: for the above code, we'd get a `Fraction` whose numerator and denominator were both of actual type `int` instead of `mpz`. Whether that's a desirable behaviour change or not is another question ...

[1] https://gmpy2.readthedocs.io/en/latest/
msg309306 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-12-31 20:38
> if math.gcd() only accepts integers and not, at least, numbers.Integral, wouldn't that be a bug?

I'd call it an enhancement opportunity rather than a bug. :-) There's no general Python-wide requirement that an instance of numbers.Integral should be acceptable anywhere an int is (though I agree that it's a nice property to have in general).

But as it happens, math.gcd _does_ accept instances of numbers.Integral, since any such instance should implement the __index__ method to convert itself to a plain old int, and math.gcd passes its arguments through PyNumber_Index.
msg309307 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-12-31 20:44
> but it would result in a behaviour change: for the above code, we'd get a `Fraction` whose numerator and denominator were both of actual type `int` instead of `mpz`

Ah, sorry. That's not true in this particular case. The returned gcd would be of type `int`, but then we end up dividing an `mpz` by an `int` to get an `mpz`. So I guess the problem case is where we have something that's like `mpz` but that doesn't support division by a plain `int`: I don't think there's any requirement that instances of numbers.Integral support mixed-type arithmetic with plain ints.

I'd actually be happier if the `fractions.Fraction` type *did* normalise so that its numerator and denominator always had exact type `int`, but that's not the way that it was designed: it was designed to support arbitrary numbers.Integral instances.
msg309309 - (view) Author: Gordon P. Hemsley (gphemsley) * Date: 2017-12-31 21:26
Side note: https://github.com/aleaxit/gmpy/issues/127 suggests that the types in question were added to the numeric tower for gmpy 2.0.9 and 2.1.0.
msg309310 - (view) Author: Gordon P. Hemsley (gphemsley) * Date: 2017-12-31 21:31
So, if I'm understanding your position correctly:

* We're back to needing a test for the line in question.
* We're eschewing the possibility of changing the behavior of `fractions.Fraction` to force int numerator and denominator.

Is that correct?
msg309329 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-01-01 09:39
> the types in question were added to the numeric tower for gmpy 2.0.9 and 2.1.0.

Ah, good to know. Thanks. I was using 2.0.8.

> * We're back to needing a test for the line in question.

Agreed. It's not currently covered, and with the current behaviour the line _is_ necessary.

> * We're eschewing the possibility of changing the behavior of `fractions.Fraction` to force int numerator and denominator.

I think that's a separate issue from this one, and if you want to pursue it it's worth its own issue on the tracker. But there would need to be a good reason for deliberately changing the existing by-design behaviour.
msg309330 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-01-01 10:00
I've retitled the issue to avoid confusion, since we're no longer proposing removing the _gcd function.
msg390771 - (view) Author: Sergey B Kirpichev (Sergey.Kirpichev) * Date: 2021-04-11 05:38
Mark, I think the Lib/fractions.py now is covered by tests in the test_fractions.py.

$ ./python -m venv ~/venv/cpython
$ . ~/venv/cpython/bin/activate
$ pip install coverage
Collecting coverage
  Using cached coverage-5.5-cp310-cp310-manylinux1_x86_64.whl (238 kB)
Installing collected packages: coverage
Successfully installed coverage-5.5
$ python -m coverage run --source=fractions --branch Lib/test/test_fractions.py
...............................
----------------------------------------------------------------------
Ran 31 tests in 0.040s

OK
$ ~/venv/cpython/bin/python -m coverage report
Name               Stmts   Miss Branch BrPart  Cover
----------------------------------------------------
Lib/fractions.py     299      0    126      0   100%
----------------------------------------------------
TOTAL                299      0    126      0   100%

My guess: this issue can be closed.
msg390772 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2021-04-11 08:05
Thanks, Sergey! Agreed that this can be closed.
History
Date User Action Args
2022-04-11 14:58:56adminsetgithub: 76647
2021-04-11 08:05:16mark.dickinsonsetstatus: open -> closed
resolution: out of date
messages: + msg390772

stage: resolved
2021-04-11 05:38:41Sergey.Kirpichevsetnosy: + Sergey.Kirpichev
messages: + msg390771
2018-01-01 10:00:01mark.dickinsonsetmessages: + msg309330
title: Remove fractions._gcd() -> Fix missing test coverage for fractions.Fraction.__new__
2018-01-01 09:39:35mark.dickinsonsetmessages: + msg309329
2017-12-31 21:31:29gphemsleysetmessages: + msg309310
2017-12-31 21:26:31gphemsleysetmessages: + msg309309
2017-12-31 20:44:08mark.dickinsonsetmessages: + msg309307
2017-12-31 20:38:10mark.dickinsonsetmessages: + msg309306
2017-12-31 20:23:32mark.dickinsonsetmessages: + msg309304
2017-12-31 19:33:10mark.dickinsonsetmessages: + msg309299
2017-12-31 18:53:51gphemsleysetmessages: + msg309297
2017-12-31 16:08:06mark.dickinsonsetmessages: + msg309290
2017-12-31 14:53:54gphemsleycreate