python / cpython

The Python programming language
https://www.python.org
Other
62.17k stars 29.89k forks source link

`denominator` of `Fraction` is positive, which should be documented #122450

Open privet-kitty opened 1 month ago

privet-kitty commented 1 month ago

Documentation

This suggestion is to clarify the current behavior of the fractions module.

The current implementation of Fraction reduces an input numerator and a denominator so that the latter is positive, which is implemented in the following places:

https://github.com/python/cpython/blob/d27a53fc02a87e76066fc4e15ff1fff3922a482d/Lib/fractions.py#L299-L303

This behavior is partially documented in https://docs.python.org/3.14/library/fractions.html:

  • numerator
    • Numerator of the Fraction in lowest term.
  • denominator
    • Denominator of the Fraction in lowest term.
  • as_integer_ratio()
    • Return a tuple of two integers, whose ratio is equal to the original Fraction. The ratio is in lowest terms and has a positive denominator.

It says that a denominator returned by as_integer_ratio is positive, but doesn't mention to the sign of the denominator property. I think there should be no problem with making that explicit also in the explanation of denominator.

Linked PRs

picnixz commented 1 month ago

This is not necessarily the case if the input is a rational (which could be a custom class where the denominator is not always positive):

https://github.com/python/cpython/blob/d27a53fc02a87e76066fc4e15ff1fff3922a482d/Lib/fractions.py#L241-L244

privet-kitty commented 1 month ago

@picnixz Thanks for the information. I didn't realize that.

So what is not true is actually the description of as_integer_ratio? This is a bad news...

picnixz commented 1 month ago

Nevermind, the online docs actually say that the denominator of a rational should be positive: https://docs.python.org/3/library/numbers.html#numbers.Rational.

So actually, we can indeed improve the docs by saying that the denominator is positive.

privet-kitty commented 1 month ago

Thank you! I'll fix it.

skirpichev commented 1 month ago

This is not necessarily the case if the input is a rational (which could be a custom class where the denominator is not always positive):

No, it couldn't. But it seems you already realized that.

So actually, we can indeed improve the docs by saying that the denominator is positive.

I'm not sure it's a real improvement as the numbers module already documents that.

Never mind, it seems that docstrings for numerator/denominator are missing. That looks as an issue. I think we should add them (to be like int.numerator/denominator).

picnixz commented 1 month ago

But it seems you already realized that.

Yes, it's just that I don't read the online docs, I usually read the docstring when I can (just because it's easier with an IDE...).

I'm not sure it's a real improvement as the numbers module already documents that.

Since it's in another module (and webpage) people might forget about it. I think it's worth the redundancy.