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

rhettinger · 2019-08-11T17:38:01Z

https://bugs.python.org/issue37819

gvanrossum

I would change "exactly equal" to just "equal" -- there's nothing to be gained by adding "exactly".

gvanrossum · 2019-08-11T18:18:21Z

Doc/library/fractions.rst

@@ -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 integers, whose ratio is exactly equal


Drop "exactly"

@gvanrossum To some degree, I think that mentioning "exactly" might help readers to understand the advantage of using an integer ratio when they need more precision. However, there might be a better way to mention this.

Thanks. I'll drop this "exactly". The wording was take from the docstring for float.as_integer_ratio() but in makes a lot more sense.

gvanrossum · 2019-08-11T18:18:29Z

Lib/fractions.py

+    def as_integer_ratio(self):
+        """Return integer ratio.
+
+        Return a pair of integers, whose ratio is exactly equal to the original


aeros

Thanks for the PR @rhettinger.

I had some minor suggestions:

aeros · 2019-08-11T18:20:49Z

Misc/NEWS.d/next/Library/2019-08-11-10-34-19.bpo-37819.LVJls-.rst

@@ -0,0 +1,2 @@
+Add Fraction.as_integer_ratio() to match the corresponding methods in bool,


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:

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

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

@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

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

@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)

@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.

aeros · 2019-08-11T18:31:09Z

Lib/fractions.py

+    def as_integer_ratio(self):
+        """Return integer ratio.
+
+        Return a pair of integers, whose ratio is exactly equal to the original


Would it be potentially helpful to readers to specify in the doc-string that the integer pair is returned as a tuple? This is not uncommonly done, but I think it's worthwhile to specifically mention this:

Suggested change

Return a pair of integers, whose ratio is exactly equal to the original

Return a tuple containing two integers, whose ratio is exactly equal to the original

aeros · 2019-08-11T18:50:24Z

Lib/test/test_fractions.py

@@ -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):


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.

No need. Fractions don't have that concept.

aeros · 2019-08-11T19:04:45Z

I didn't include this in my review since it's not directly related to the PR, but I noticed some minor inconsistencies in the tests between the different versions of *.as_integer_ratio(). For example, the test function in float was called test_floatasratio() instead of test_as_integer_ratio() (which is used by decimal.py and now in this PR), and it also seemed to be missing the test that decimal uses to ensure a ValueError is raised when attempting to use an invalid string:

       self.assertRaises(ValueError,
                          Decimal.as_integer_ratio, Decimal('snan123'))

Also, as far as I can tell, there's no test coverage at all for the int version of the method. Should I open a new issue for this?

* The other methods spell fraction with a capital F. * The word "original" doesn't make sense because the original fraction is reduced: Fraction(4, 6).as_integer_ratio() |-> (2, 3)

miss-islington · 2019-08-11T21:41:02Z

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

(cherry picked from commit f03b4c8) Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>

bedevere-bot · 2019-08-11T21:41:11Z

GH-15215 is a backport of this pull request to the 3.8 branch.

(cherry picked from commit f03b4c8) Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>

jdemeyer · 2019-08-13T09:24:01Z

Should this as_integer_ratio instead be moved to the numbers.Rational base class? The implementation already makes sense on that level, so it seems more natural to do it that way.

bpo-37819: Add Fraction.as_integer_ratio()

3ca5886

rhettinger added type-feature A feature request or enhancement needs backport to 3.8 labels Aug 11, 2019

rhettinger requested review from mdickinson and gvanrossum August 11, 2019 17:38

the-knights-who-say-ni added the CLA signed label Aug 11, 2019

bedevere-bot added the awaiting core review label Aug 11, 2019

gvanrossum reviewed Aug 11, 2019

View reviewed changes

aeros reviewed Aug 11, 2019

View reviewed changes

rhettinger added 2 commits August 11, 2019 14:04

Address reviewer comments

b521c42

Minor word-smithing.

b55d9d8

* The other methods spell fraction with a capital F. * The word "original" doesn't make sense because the original fraction is reduced: Fraction(4, 6).as_integer_ratio() |-> (2, 3)

rhettinger merged commit f03b4c8 into python:master Aug 11, 2019

rhettinger deleted the fraction_ratio branch August 11, 2019 21:41

bedevere-bot removed the awaiting core review label Aug 11, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 11, 2019

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

8a97a86

(cherry picked from commit f03b4c8) Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>

bedevere-bot removed the needs backport to 3.8 label Aug 11, 2019

rhettinger added a commit that referenced this pull request Aug 11, 2019

bpo-37819: Add Fraction.as_integer_ratio() (GH-15212) (GH-15215)

5ba1cb0

(cherry picked from commit f03b4c8) Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>

aeros mentioned this pull request Aug 19, 2019

bpo-37884: Optimize Fraction() and statistics.mean() #15329

Closed

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

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

d738b5c

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

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

476a8a4

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

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

91441a2

skirpichev mentioned this pull request Feb 12, 2023

gh-101825: mention, that as_integer_ratio() output is normalized #101843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rhettinger commented Aug 11, 2019 •

edited by bedevere-bot

Loading

gvanrossum left a comment

gvanrossum Aug 11, 2019

aeros Aug 11, 2019

rhettinger Aug 11, 2019

gvanrossum Aug 11, 2019

rhettinger Aug 11, 2019

aeros left a comment

aeros Aug 11, 2019

rhettinger Aug 11, 2019

aeros Aug 11, 2019 •

edited

Loading

aeros Aug 11, 2019 •

edited

Loading

methane Aug 19, 2019

aeros Aug 19, 2019

aeros Aug 11, 2019 •

edited

Loading

rhettinger Aug 11, 2019

aeros Aug 11, 2019 •

edited

Loading

rhettinger Aug 11, 2019

aeros commented Aug 11, 2019 •

edited

Loading

miss-islington commented Aug 11, 2019

bedevere-bot commented Aug 11, 2019

jdemeyer commented Aug 13, 2019

		@@ -0,0 +1,2 @@
		Add Fraction.as_integer_ratio() to match the corresponding methods in bool,

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

	Return a pair of integers, whose ratio is exactly equal to the original
	Return a tuple containing two integers, whose ratio is exactly equal to the original

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

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

Conversation

rhettinger commented Aug 11, 2019 • edited by bedevere-bot Loading

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

aeros Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeros commented Aug 11, 2019 • edited Loading

miss-islington commented Aug 11, 2019

bedevere-bot commented Aug 11, 2019

jdemeyer commented Aug 13, 2019

rhettinger commented Aug 11, 2019 •

edited by bedevere-bot

Loading

aeros Aug 11, 2019 •

edited

Loading

aeros Aug 11, 2019 •

edited

Loading

aeros Aug 11, 2019 •

edited

Loading

aeros Aug 11, 2019 •

edited

Loading

aeros commented Aug 11, 2019 •

edited

Loading