Issue32466

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.

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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | Date: 2021-04-11 08:05 | |

Thanks, Sergey! Agreed that this can be closed. |

History | |||
---|---|---|---|

Date | User | Action | Args |

2022-04-11 14:58:56 | admin | set | github: 76647 |

2021-04-11 08:05:16 | mark.dickinson | set | status: open -> closed resolution: out of date messages: + msg390772 stage: resolved |

2021-04-11 05:38:41 | Sergey.Kirpichev | set | nosy:
+ Sergey.Kirpichev messages: + msg390771 |

2018-01-01 10:00:01 | mark.dickinson | set | messages:
+ msg309330 title: Remove fractions._gcd() -> Fix missing test coverage for fractions.Fraction.__new__ |

2018-01-01 09:39:35 | mark.dickinson | set | messages: + msg309329 |

2017-12-31 21:31:29 | gphemsley | set | messages: + msg309310 |

2017-12-31 21:26:31 | gphemsley | set | messages: + msg309309 |

2017-12-31 20:44:08 | mark.dickinson | set | messages: + msg309307 |

2017-12-31 20:38:10 | mark.dickinson | set | messages: + msg309306 |

2017-12-31 20:23:32 | mark.dickinson | set | messages: + msg309304 |

2017-12-31 19:33:10 | mark.dickinson | set | messages: + msg309299 |

2017-12-31 18:53:51 | gphemsley | set | messages: + msg309297 |

2017-12-31 16:08:06 | mark.dickinson | set | messages: + msg309290 |

2017-12-31 14:53:54 | gphemsley | create |