Skip to content
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.

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

bpo-33073: Adding as_integer_ratio to ints. #8750

Merged
merged 18 commits into from Sep 14, 2018

Conversation

@lisroach
Copy link
Contributor

commented Aug 12, 2018

Adding as_integer_ratio to ints to make them more interoperable with floats.

https://bugs.python.org/issue33073

PyObject *result_pair = NULL;

denominator = PyLong_FromLong(1);
result_pair = PyTuple_Pack(2, self, denominator);

This comment has been minimized.

Copy link
@pablogsal

pablogsal Aug 13, 2018

Member

If PyLong_FromLong returns NULL then PyTuple_Pack and Py_DECREF(denominator) will fail.

This comment has been minimized.

Copy link
@sir-sigurd

sir-sigurd Aug 13, 2018

Contributor

I think the whole function can be replaced with return PyTuple_Pack(2, self, _PyLong_One).

This comment has been minimized.

Copy link
@eamanu

eamanu Aug 14, 2018

Contributor

@pablogsal But PyLong_FromLong always receive the 1. How could it fail?

This comment has been minimized.

Copy link
@pablogsal

pablogsal Aug 14, 2018

Member

The contract of PyLong_FromLong says that it can return NULL on failure. Even if the current implementation (that uses the array of small ints) makes it improbable/impossible to fail, this can change in the future without changing the external API.

Return a pair of integers, whose ratio is exactly equal to the original int
and with a positive denominator.
Raise OverflowError on infinities and a ValueError on NaNs.

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Aug 13, 2018

Contributor

An int can never be any of these things - remove this line

This comment has been minimized.

Copy link
@eamanu

eamanu Aug 14, 2018

Contributor

Yes. math.inf is float class

def test_as_integer_ratio(self):
tests = [10, 0, -10, 1, 3]
for value in tests:
self.assertEqual((value).as_integer_ratio(), (value, 1))

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Aug 13, 2018

Contributor

nit: no parens needed around value

Return a pair of integers whose ratio is exactly equal to the original integer
and with a positive denominator. The integer ratio of integers (whole numbers)
is always the integer as the numerator and 1 as the denominator.

This comment has been minimized.

Copy link
@ZackerySpytz

ZackerySpytz Aug 14, 2018

Contributor

A versionadded directive needs to be added. Also, the developer's guide states that reST files should use an indentation of 3 spaces.

int_as_integer_ratio_impl(PyObject *self)
/*[clinic end generated code: output=e60803ae1cc8621a input=ce9c7768a1287fb9]*/
{
return PyTuple_Pack(2, self, _PyLong_One)

This comment has been minimized.

Copy link
@mdickinson

mdickinson Aug 17, 2018

Member

There's a missing semicolon at the end of this line; I think that's why the continuous integration builds are failing.

Also, please change the indentation of this line to 4 spaces instead of 2 spaces, following PEP7. Thanks!

lisroach and others added 2 commits Aug 22, 2018
@mdickinson

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

@lisroach Thanks for the update. I've pushed a commit that should fix the failing test_doctest.

@mdickinson
Copy link
Member

left a comment

@lisroach If you're okay with my recent updates, I think this PR is ready to merge.

int_as_integer_ratio_impl(PyObject *self)
/*[clinic end generated code: output=e60803ae1cc8621a input=c1aea0aa6fb85c28]*/
{
return PyTuple_Pack(2, self, _PyLong_One);

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 25, 2018

Member

True.as_integer_ratio() will return (True, 1). It should return (1, 1).

This comment has been minimized.

Copy link
@mdickinson

mdickinson Aug 25, 2018

Member

Hmm; good point. There's a precedent here in the form of True.numerator, which returns 1.

Return a pair of integers, whose ratio is exactly equal to the original int
and with a positive denominator.
>>> (10).as_integer_ratio()

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 25, 2018

Member

Do we need so much examples in a docstring?

This comment has been minimized.

Copy link
@mdickinson

mdickinson Aug 25, 2018

Member

I think it's useful to have at least one positive and one negative example, so that it's obvious at a glance that the behaviour for negatives is to give (for example) (-5, 1) rather than (5, -1). I could imagine users thinking that 0 was somehow a special case, too, so I like that we have the 0 example there.

@@ -1349,6 +1349,11 @@ def test_shift_bool(self):
self.assertEqual(type(value << shift), int)
self.assertEqual(type(value >> shift), int)

def test_as_integer_ratio(self):
tests = [10, 0, -10, 1, 3]

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 25, 2018

Member

Why so much similar cases are needed?

Add tests for booleans and other int subclasses. Check types of numerator and denominator.


Return a pair of integers whose ratio is exactly equal to the original
integer and with a positive denominator. The integer ratio of integers
(whole numbers) is always the integer as the numerator and 1 as the

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 25, 2018

Member

``1``

@mdickinson
Copy link
Member

left a comment

Changing my approval due to the issues Serhiy noted. @lisroach do you want to tackle the outstanding comments? I'm happy to pick this up if not.

@bedevere-bot

This comment has been minimized.

Copy link

commented Aug 25, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@lisroach

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

I have made the requested changes; please review again.

I am not sure if I could do the boolean check better- somehow in one line? I'm open to advice!

Thanks for all the reviews @mdickinson and @serhiy-storchaka it's been really helpful!

class Foo(enum.IntEnum):
X = 42
self.assertEqual(Foo.X.as_integer_ratio(), (42, 1))
assert(type(Foo.X.as_integer_ratio()[0] == int))

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 13, 2018

Contributor

This should be: self.assertEqual(type(Foo.X.as_integer_ratio()[0], int). Unittest doesn't use assertion statements.

return PyTuple_Pack(2, self, _PyLong_One);
else {
PyObject *temp = PyNumber_Positive(self);
Py_DECREF(temp);

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 13, 2018

Contributor

The DECREF needs to occur after building the tuple; otherwise, the Python integer object can (and likely will) disappear before it gets used.

if PyLong_CheckExact(self)
return PyTuple_Pack(2, self, _PyLong_One);
else {
PyObject *temp = PyNumber_Positive(self);

This comment has been minimized.

Copy link
@rhettinger

rhettinger Sep 13, 2018

Contributor

We need another way to do this. The intent of this code is to construct a regular integer instance from an instance of an int subclass. The problem with PyNumber_Positive() is that the subclass can itself define pos() to return something other than an exact int. Elsewhere, we use _PyLong_Copy for this purpose.

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Sep 14, 2018

Contributor

Is there a reason why PyNumber_Index does not call _PyLong_Copy?

@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 13, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Am checking to see the Serhiy's issues are resolved.

rhettinger and others added 2 commits Sep 14, 2018
@eric-wieser

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

Reviving a comment hidden away above - should operator.index / PyNumber_Index / int->tp_as_number->nb_index call _PyLong_Copy too for consistency?

{
if PyLong_CheckExact(self) {
return PyTuple_Pack(2, self, _PyLong_One);
} else {

This comment has been minimized.

Copy link
@sir-sigurd

sir-sigurd Sep 14, 2018

Contributor

else { should be on the next line per PEP 7.

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Sep 14, 2018

Contributor

Missing parens on the if too

@rhettinger rhettinger merged commit 5ac7043 into python:master Sep 14, 2018

9 checks passed

Linux-PR #Linux-PR_20180914.24 succeeded
Details
Linux-PR-Coverage #Linux-PR-Coverage_20180914.20 succeeded
Details
Windows-PR #Windows-PR_20180914.24 succeeded
Details
bedevere/issue-number Issue number 33073 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docs #docs_20180914.24 succeeded
Details
macOS-PR #macOS-PR_20180914.24 succeeded
Details
@eric-wieser

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

ISTM, the code is reasonable as-is.

Despite the missing null check and invalid C syntax that works only because of a macro?

@rhettinger

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

Eric, I didn't see your comments prior to merging. See PR 9297 for the cleanup. The part that was fine as-is was using PyTuple_Pack() on two different code paths.

@eric-wieser

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

Agreed, the two code paths thing was unimportant. #9297 seems to address all my comments, other than the question about nb_index, which is tangential anyway

@rhettinger

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

Can you note your review on 9297 please.

@eric-wieser

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

If that's aimed at me, I don't know what you're asking me to do.

@serhiy-storchaka
Copy link
Member

left a comment

This PR was merged too soon. There are several issues with it. See also comments by @sir-sigurd and @eric-wieser.

@@ -1365,20 +1365,20 @@ def test_as_integer_ratio_maxint(self):
def test_as_integer_ratio_bool(self):
self.assertEqual(True.as_integer_ratio(), (1, 1))
self.assertEqual(False.as_integer_ratio(), (0, 1))
assert(type(True.as_integer_ratio()[0]) == int)
assert(type(False.as_integer_ratio()[0]) == int)
self.assertEqual(type(True.as_integer_ratio()[0]), int)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Sep 14, 2018

Member

Test also the denumerator type.

if PyLong_CheckExact(self) {
return PyTuple_Pack(2, self, _PyLong_One);
} else {
PyObject *numerator = _PyLong_Copy(self);

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Sep 14, 2018

Member

Use the same same code as for for the numerator getter.

Test the result for NULL.

@@ -91,6 +91,10 @@ Other Language Changes
was lifted.
(Contributed by Serhiy Storchaka in :issue:`32489`.)

* The ``int`` type now has a new ``as_integer_ratio`` method compatible

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Sep 14, 2018

Member

Add links:

:class:`int`
:meth:`~int.as_integer_ratio`
:meth:`float.as_integer_ratio`
@serhiy-storchaka

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

Reviving a comment hidden away above - should operator.index / PyNumber_Index / int->tp_as_number->nb_index call _PyLong_Copy too for consistency?

This is a different issue. And I think that it should not.

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.