Skip to content

bpo-37819: Add Fraction.as_integer_ratio() #15212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 11, 2019
Merged

bpo-37819: Add Fraction.as_integer_ratio() #15212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ca5886
bpo-37819: Add Fraction.as_integer_ratio()
rhettinger Aug 11, 2019
b521c42
Address reviewer comments
rhettinger Aug 11, 2019
b55d9d8
Minor word-smithing.
rhettinger Aug 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Doc/library/fractions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ another rational number, or from a string.
Denominator of the Fraction in lowest term.


.. method:: as_integer_ratio()

Return a tuple of two integers, whose ratio is equal
to the Fraction and with a positive denominator.

.. versionadded:: 3.8

.. method:: from_float(flt)

This class method constructs a :class:`Fraction` representing the exact
Expand Down
8 changes: 8 additions & 0 deletions Lib/fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,14 @@ def from_decimal(cls, dec):
(cls.__name__, dec, type(dec).__name__))
return cls(*dec.as_integer_ratio())

def as_integer_ratio(self):
"""Return the integer ratio as a tuple.

Return a tuple of two integers, whose ratio is equal to the
Fraction and with a positive denominator.
"""
return (self._numerator, self._denominator)

def limit_denominator(self, max_denominator=1000000):
"""Closest Fraction to self with denominator at most max_denominator.

Expand Down
6 changes: 6 additions & 0 deletions Lib/test/test_fractions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,12 @@ def testFromDecimal(self):
ValueError, "cannot convert NaN to integer ratio",
F.from_decimal, Decimal("snan"))

def test_as_integer_ratio(self):
Copy link
Contributor

@aeros aeros Aug 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a comparison of these tests to the existing ones, and noticed that test_decimal.py contains coverage for attempting to convert inf (inf = 1e1000), -inf, nan (nan = inf - inf), and an invalid string to an integer ratio:

    def test_as_integer_ratio(self):
        Decimal = self.decimal.Decimal

        # exceptional cases
        self.assertRaises(OverflowError,
                          Decimal.as_integer_ratio, Decimal('inf'))
        self.assertRaises(OverflowError,
                          Decimal.as_integer_ratio, Decimal('-inf'))
        self.assertRaises(ValueError,
                          Decimal.as_integer_ratio, Decimal('-nan'))
        self.assertRaises(ValueError,
                          Decimal.as_integer_ratio, Decimal('snan123'))

For the sake of consistency, should we include that here as well? I'm not certain that it's necessary, but I figured it was worth mentioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need. Fractions don't have that concept.

self.assertEqual(F(4, 6).as_integer_ratio(), (2, 3))
self.assertEqual(F(-4, 6).as_integer_ratio(), (-2, 3))
self.assertEqual(F(4, -6).as_integer_ratio(), (-2, 3))
self.assertEqual(F(0, 6).as_integer_ratio(), (0, 1))

def testLimitDenominator(self):
rpi = F('3.1415926535897932')
self.assertEqual(rpi.limit_denominator(10000), F(355, 113))
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2019-08-11-10-34-19.bpo-37819.LVJls-.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add Fraction.as_integer_ratio() to match the corresponding methods in bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be helpful to provide a link to the method using inline markup? This would allow readers to easily find the documentation for the new method.

Also regarding this line, as far as I'm aware, there's not a bool.as_integer_ratio() method. To confirm this, I used git grep to search the documentation:

image

As expected, it returned the methods for decimal, int, and float but nothing for bool.

Suggested change
Add Fraction.as_integer_ratio() to match the corresponding methods in bool,
Add :meth:`Fraction.as_integer_ratio` to match the corresponding methods in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to provide cross-links from Misc/NEWS. It is okay for entries there to be plain text.

Copy link
Contributor

@aeros aeros Aug 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhettinger Oh yeah it's okay either way, but since the Misc/NEWS entries support reST and Sphinx I think it's helpful to include links to functions, methods, or classes that were modified in functionality or documentation. Usually the news entries are very brief, so if a curious reader wants to get more information they can simply use the link.

To those of us who are familiar with the structuring of the documentation, it's usually easy to find the relevant sections. But, some less familiar or newer readers might have trouble doing so, especially if they use an external search engine (which frequently will lead them to the 2.7 docs or an incorrect page).

It's more a convenience/QoL addition for the readers than anything, and I don't think it hurts the readability or takes much effort from our end. I might start a thread over on python-dev to see what the others think. Not at all for this PR specifically, but just in general.

Edit: Just realized that I can't start threads on the Python-Dev list. I'll open a new topic on Discuss instead, let me know if you'd like a link to the thread. Never mind, I'm able to create a new thread on the ML. I'll do so later tonight, in the meantime here's the discuss post: https://discuss.python.org/t/should-news-entries-contain-documentation-links/2127

Copy link
Contributor

@aeros aeros Aug 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what was the conclusion on bool being in the news entry? From my understanding, there is not a bool.as_integer_ratio() method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeros167 Have you validated your understanding?

From my understanding, bool inherits from int. If int has as_integer_ratio(), bool should have it too.

$ ./python
Python 3.9.0a0 (heads/master:e95ac20103, Jul 24 2019, 21:48:01)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> i = 42
>>> i.as_integer_ratio()
(42, 1)
>>> True.as_integer_ratio()
(1, 1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@methane Thanks for the explanation!

At first this didn't make sense to me, but then I remembered that:

>>> int(True)
1
>>> int(False)
0
>>> True == 1
True
>>> False == 0
True

So it makes sense that the boolean values would also have the int methods, since they are effectively specialized ints. I was just confused at first since I could not find an explicit mention of bool.is_integer_ratio() in the docs, but I suppose it doesn't need to be mentioned since as you said, bool inherits from int.

int, float, and decimal.