gh-122450: Fix docs to state denominator positivity of Fraction by privet-kitty · Pull Request #122464 · python/cpython

privet-kitty · 2024-07-30T14:26:53Z

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

📚 Documentation preview 📚: https://cpython-previews--122464.org.readthedocs.build/

ghost · 2024-07-30T14:26:55Z

All commit authors signed the Contributor License Agreement.

skirpichev

There is some redundancy, as docs already says: "The Fraction class inherits from the abstract base class numbers.Rational, and implements all of the methods and operations from that class." and "The numerator and denominator values should be instances of Integral and should be in lowest terms with denominator positive." But I think it's ok.

On another hand, it seems that docstrings for above properties are missing. I think this can be added in this pr. Probably, we shouldn't be too verbose here and docstrings could be as:

>>> help(int.numerator)
Help on getset descriptor builtins.int.numerator:

numerator
    the numerator of a rational number in lowest terms

>>> help(int.denominator)
Help on getset descriptor builtins.int.denominator:

denominator
    the denominator of a rational number in lowest terms

>>>

Doc/library/fractions.rst

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

privet-kitty · 2024-07-31T12:05:44Z

@skirpichev Thanks for your review.

Fix rst according to your suggestion
Add docstrings to numerator and denominator of Fraction

skirpichev

I think we should be more close to int docstrings.

CC @picnixz

Lib/fractions.py

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

picnixz

We can use a more imperative style like this. And yes we should match the docstring formulation of int, though I think we can add "(positive)" since for ints, the denominator is always "1".

Lib/fractions.py

Doc/library/fractions.rst

picnixz

By the way, you could also add the docstring at the level of numbers.Rational.denominator rather than at the level of fractions.Fraction.denominator. Because the positivity of the denominator is for any subclass of Rational AFAIK (Fraction is a subclass of Rational)

skirpichev · 2024-07-31T13:43:19Z

add the docstring at the level of numbers.Rational.denominator rather than at the level of fractions.Fraction.denominator. Because the positivity of the denominator is for any subclass of Rational

Good point. numbers.Rational assumes a specific canonical form for numerator/denominator (not just being positive).

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

privet-kitty · 2024-08-01T14:10:20Z

@picnixz @skirpichev Thanks. I reflected the suggestions and added docstrings to numerator and denominator of Rational

Lib/fractions.py

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

privet-kitty · 2024-08-02T02:59:01Z

@skirpichev thanks, reflected

StanFromIreland

I propose "expressed in its lowest terms" rather than "in lowest terms" for clarity.

What do you think @skirpichev ?

StanFromIreland · 2025-01-18T12:26:39Z

Doc/library/fractions.rst

   .. attribute:: numerator

-      Numerator of the Fraction in lowest term.
+      Numerator of the Fraction in lowest terms.


Suggested change

Numerator of the Fraction in lowest terms.

Numerator of the Fraction, expressed in its lowest terms.

StanFromIreland · 2025-01-18T12:26:57Z

Doc/library/fractions.rst


-      Denominator of the Fraction in lowest term.
-
+      Positive denominator of the Fraction in lowest terms.


Suggested change

Positive denominator of the Fraction in lowest terms.

Positive denominator of the Fraction, expressed in its lowest terms.

StanFromIreland · 2025-01-18T12:27:29Z

Lib/numbers.py

    @property
    @abstractmethod
    def numerator(self):
+        """The numerator of a rational number in lowest terms."""


Suggested change

"""The numerator of a rational number in lowest terms."""

"""The numerator of a rational number, expressed in its lowest terms."""

StanFromIreland · 2025-01-18T12:27:52Z

Lib/numbers.py

    @property
    @abstractmethod
    def denominator(self):
+        """The (positive) denominator of a rational number in lowest terms."""


Suggested change

"""The (positive) denominator of a rational number in lowest terms."""

"""The (positive) denominator of a rational number, expressed in its lowest terms."""

skirpichev · 2025-01-18T12:54:04Z

I propose "expressed in its lowest terms" rather than "in lowest terms" for clarity.

I think current wording is fine.

python-cla-bot · 2025-04-18T09:35:19Z

All commit authors signed the Contributor License Agreement.

skirpichev · 2025-07-19T11:15:11Z

Doc/library/fractions.rst

   .. attribute:: numerator

-      Numerator of the Fraction in lowest term.
+      Numerator of the Fraction in lowest terms.


In fact, we say that "The Fraction class inherits from the abstract base class numbers.Rational, and implements all of the methods and operations from that class."

So, we could move these numerator/denominator descriptions to the Rational class.

CC @AA-Turner

That would be worse; it requires people to look in two places to find out what happens with Fraction. Happy to improve in both places, but we shouldn't remove from here.

This pr continued in #136800. I hope, I'll address review quickly;-)

but we shouldn't remove from here.

I'm not sure if it worth verbosity. I don't see e.g. where described real/imag properties for builtin types, except for the numbers module.

@skirpichev should this PR (#122464) be closed?

A

I think so. I tried to attribute this work in a follow-up pr.

AA-Turner · 2025-07-21T12:17:33Z

Closing: see #136800.

A

Fix manual to state denominator positivity of Fraction

04056ff

bedevere-app bot added the docs Documentation in the Doc dir label Jul 30, 2024

bedevere-app bot mentioned this pull request Jul 30, 2024

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

Open

bedevere-app bot added skip news awaiting review labels Jul 30, 2024

skirpichev approved these changes Jul 31, 2024

View reviewed changes

Doc/library/fractions.rst Outdated Show resolved Hide resolved

bedevere-app bot added awaiting core review and removed awaiting review labels Jul 31, 2024

privet-kitty and others added 3 commits July 31, 2024 20:57

Update Doc/library/fractions.rst

e5a4608

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Add docstrings to numerator and denominator of Fraction

05c764d

Correct a word

ad66afb

skirpichev reviewed Jul 31, 2024

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

Lib/fractions.py Outdated Show resolved Hide resolved

privet-kitty and others added 2 commits July 31, 2024 21:57

Update Lib/fractions.py

416b9cd

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Update Lib/fractions.py

7cb1905

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

picnixz reviewed Jul 31, 2024

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

Doc/library/fractions.rst Outdated Show resolved Hide resolved

picnixz reviewed Jul 31, 2024

View reviewed changes

privet-kitty and others added 3 commits August 1, 2024 23:06

Update Lib/fractions.py

ddec59c

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Update Doc/library/fractions.rst

ed48fb7

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Add docstrings to numerator and denominator of Rational

c575b47

skirpichev reviewed Aug 2, 2024

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

Lib/fractions.py Outdated Show resolved Hide resolved

privet-kitty and others added 2 commits August 2, 2024 11:58

Update Lib/fractions.py

a531cb6

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Update Lib/fractions.py

1f2519a

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

Wulian233 approved these changes Oct 6, 2024

View reviewed changes

StanFromIreland reviewed Jan 18, 2025

View reviewed changes

skirpichev reviewed Jul 19, 2025

View reviewed changes

skirpichev mentioned this pull request Jul 19, 2025

gh-122450: Indicate that Fraction denominators are always positive #136789

Merged

Merge branch 'main' into feature/denominator-positivity

2e07bfc

AA-Turner closed this Jul 21, 2025

	Numerator of the Fraction in lowest terms.
	Numerator of the Fraction, expressed in its lowest terms.


		Denominator of the Fraction in lowest term.

		Positive denominator of the Fraction in lowest terms.

	Positive denominator of the Fraction in lowest terms.
	Positive denominator of the Fraction, expressed in its lowest terms.

	"""The numerator of a rational number in lowest terms."""
	"""The numerator of a rational number, expressed in its lowest terms."""

	"""The (positive) denominator of a rational number in lowest terms."""
	"""The (positive) denominator of a rational number, expressed in its lowest terms."""

Uh oh!

Conversation

privet-kitty commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 30, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

privet-kitty commented Jul 31, 2024

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Jul 31, 2024

Uh oh!

privet-kitty commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

privet-kitty commented Aug 2, 2024

Uh oh!

StanFromIreland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Jan 18, 2025

Uh oh!

python-cla-bot bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AA-Turner commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

privet-kitty commented Jul 30, 2024 •

edited

Loading

ghost commented Jul 30, 2024 •

edited by ghost

Loading

picnixz left a comment •

edited

Loading

python-cla-bot bot commented Apr 18, 2025 •

edited

Loading

AA-Turner commented Jul 21, 2025 •

edited

Loading