bpo-26680: Incorporate is_integer in all built-in and standard library numeric types #6121

rob-smallshire · 2018-03-15T10:39:20Z

This change implements support for the x.is_integer() predicate across all built-in and standard library concrete numeric types: int, bool, Fraction and Decimal; previously it was supported only on float. It also incorporates is_integer() into the abstract types Real, Rational and Integral, with appropriate default implementations at each level.

Updates to the relevant documentation and test suites are included.

https://bugs.python.org/issue26680

skrah · 2019-03-26T10:34:34Z

This issue is probably languishing because of the amount of controversy it generated. Was there a pronouncement from Guido on the mailing lists?

Regardless, it would be nice to get at least +-0 from @mdickinson @rhettinger @tim-one. I'm +1 on this.

rob-smallshire · 2019-03-26T10:42:21Z

@skrah Yes. Guido gave a pronouncement on python-dev.

mdickinson · 2019-03-26T15:16:47Z

Regardless, it would be nice to get at least +-0 from @mdickinson @rhettinger @tim-one.

+1 in principle. I haven't reviewed the code changes, and am unlikely to have the bandwidth to do so in the near future.

rob-smallshire · 2019-04-07T10:44:14Z

@skrah Is there anything I can do to help move this along?

skrah · 2019-04-07T17:14:33Z

@rob-smallshire This is basically just waiting for review now that @mdickinson is also in favor.

I probably have time to review this before the beta-1 release, which is 2019-05-27.

mdickinson

Mostly LGTM; a few comments and suggestions.

We'll need a "whatsnew" entry, and we're missing some ".. versionchanged: " notes in the documentation updates.

mdickinson · 2019-05-12T09:29:33Z

Objects/longobject.c

@@ -5237,6 +5237,12 @@ long_round(PyObject *self, PyObject *args)
    return result;
 }

+static PyObject *


Could we convert this to Argument Clinic? It seems somewhat trivial, but one advantage is having the docstring close to the definition. Another is having docstring consistency between int.is_integer and float.is_integer.

mdickinson · 2019-05-12T09:45:30Z

Lib/_pydecimal.py

+        """Returns True is self is finite and integral, otherwise False."""
+        if self._is_special:
+            return False
+        return self.to_integral_value(ROUND_FLOOR) == self


I suggest following the usual convention for optional arguments and passing the rounding mode by name (rounding=ROUND_FLOOR).

mdickinson · 2019-05-12T09:50:42Z

Lib/numbers.py

@@ -242,6 +242,17 @@ def __le__(self, other):
        """self <= other"""
        raise NotImplementedError

+    def is_integer(self):
+        """Does this Real represent an integral value?


Please can we replace this with something that isn't a question? Something like the other docstrings ("Return True if the operand is integral; otherwise return False.") would be fine. (Also applies to Rational.is_integer below.)

mdickinson · 2019-05-12T09:54:06Z

Lib/test/test_decimal.py

@@ -4346,6 +4356,13 @@ def test_implicit_context(self):
            self.assertTrue(Decimal("-1").is_signed())
            self.assertTrue(Decimal("0").is_zero())
            self.assertTrue(Decimal("0").is_zero())
+            self.assertTrue(Decimal("-1").is_integer())


Would it be worth adding a couple of tests for cases where the exponent isn't 0, e.g. Decimal("1e2") and Decimal("100e-2")? We should also test the behaviour for signalling NaNs

Ideally, we'd also test that no Decimal floating-point flags are ever raised. An easy way to do this would be to add some testcases to Lib/test/decimaltestdata/extra.decTest, which will check that exactly the flags mentioned are raised for each given testcase.

mdickinson · 2019-05-12T10:08:32Z

Doc/library/decimal.rst

+
+      Return :const:`True` if the argument is a finite integral value and
+      :const:`False` otherwise.
+


Please add a .. versionadded:: 3.10 note here and in the other relevant bits of documentation.

EDIT: edited the comment to update the version; the original suggestion of 3.8 is obviously out of date

bedevere-bot · 2019-05-12T10:09:32Z

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.

rob-smallshire · 2019-08-22T07:30:43Z

I haven't had capacity to do the requisite changes for 3.8. I propose going for 3.9.

csabella · 2020-05-22T23:43:11Z

@rob-smallshire, please address @mdickinson's review comments. Once everything looks good, it will make it into whichever release is master at the time, which right now would be 3.10. Thanks!

rob-smallshire · 2020-05-24T16:05:24Z

@csabella Will do. I've scheduled some time to dedicate to this in early June.

eric-wieser · 2020-08-20T14:07:00Z

@rob-smallshire, any plans to return?

rob-smallshire · 2020-08-20T15:25:38Z

@eric-wieser Yes, absolutely. I've been looking at the required changes. I'll be pushing updates soon.

rob-smallshire · 2020-09-17T11:58:43Z

Status update: rebased to resume work on the outstanding issues.

mdickinson · 2020-09-18T08:55:42Z

Status update: rebased to resume work on the outstanding issues.

Thanks! Please ping me when you're done with updates and ready for re-review.

…loat.is_integer(). The int.is_integer() method always returns True.

…integer() are always True.

eric-wieser · 2020-09-18T09:20:39Z

Lib/numbers.py

@@ -290,6 +301,10 @@ def __float__(self):
        """
        return self.numerator / self.denominator

+    def is_integer(self):
+        """Does this Rational represent an integral value?"""
+        return self.denominator == 1


Must denominator and numerator always be fully reduced?

If not,

Suggested change

return self.denominator == 1

return self.numerator % self.denominator == 0

Yes, self.numerator and self.denominator will always be relatively prime in normal use (and denominator will always be positive). Other parts of the fractions module assume this, so it's safe to just check self.denominator == 1 here.

This isn't about Fraction and its implementation of the Rational API though - this is about whether the API mandates the implementation behaves this way.

Ah. I see the doc is """.numerator and .denominator should be in lowest terms.""" - so perhaps worth clarifying that denominator should always be positive too.

Yes, that docstring could definitely be improved. Apart from that clarification, we could do with a line or two describing what the Rational class is actually for, rather than launching straight into a detail. But that's a job for a separate PR.

… conversion to int. This default implementation is intended to reduce the workload for subclass implementers. It is not robust in the presence of infinities or NaNs and may have suboptimal performance for other types.

…ator is one. This implementation assumes the Rational is represented in it's lowest form, as required by the class docstring.

…t and float. The call x.is_integer() is now listed in the table of operations which apply to all numeric types except complex, with a reference to the full documentation for Real.is_integer(). Mention of is_integer() has been removed from the section 'Additional Methods on Float'. The documentation for Real.is_integer() describes its purpose, and mentions that it should be overridden for performance reasons, or to handle special values like NaN.

The C implementation of Decimal already implements and uses mpd_isinteger internally, we just expose the existing function to Python. The Python implementation uses internal conversion to integer using to_integral_value(). In both cases, the corresponding context methods are also implemented. Tests and documentation are included.

rob-smallshire · 2020-09-18T15:19:53Z

@mdickinson I believe I now have made all the changes you requested, and all checks are passing.

mdickinson

LGTM. All my comments are addressed.

mdickinson · 2020-09-19T09:03:42Z

Doc/library/decimal.rst

@@ -1215,6 +1222,13 @@ In addition to the three supplied contexts, new contexts can be created with the
      Returns ``True`` if *x* is infinite; otherwise returns ``False``.


+   .. method:: is_integer(x)
+
+      Returns ``True`` if *x* is finite and integral; otherwise


One day it would be nice to fix all these docstrings for consistency (both with one another and with PEP 257). But not today.

Indeed, when you hop around the code you notice how different they all are. I tried to go for local consistency rather than global consistency.

mdickinson · 2020-09-19T17:28:05Z

I'll leave this open for a couple of days in case @rhettinger or @skrah has further comments.

rob-smallshire · 2020-10-01T13:18:02Z

Anything I can do to help get this merged @mdickinson ?

mdickinson · 2020-10-01T16:29:39Z

Anything I can do to help get this merged @mdickinson ?

Apart from pinging me in case I've forgotten? Nothing else I can think of ...

Merging shortly. Thank you!

bedevere-bot · 2020-10-01T16:40:49Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 RHEL7 3.x has failed when building commit 58a7da9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/539/builds/133) and take a look at the build logs.
Check if the failure is related to this commit (58a7da9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/539/builds/133

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

408 tests OK.

10 slowest tests:

test_unparse: 3 min 56 sec
test_concurrent_futures: 3 min 53 sec
test_capi: 3 min 7 sec
test_tokenize: 2 min 57 sec
test_multiprocessing_spawn: 2 min 37 sec
test_peg_generator: 2 min 33 sec
test_asyncio: 2 min 27 sec
test_lib2to3: 2 min 3 sec
test_multiprocessing_forkserver: 1 min 34 sec
test_unicodedata: 1 min 25 sec

1 test altered the execution environment:
test_asyncio

14 tests skipped:
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_ossaudiodev test_startfile test_tix test_tk test_ttk_guionly
test_winconsoleio test_winreg test_winsound test_zipfile64

Total duration: 8 min 3 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/sslproto.py", line 321, in __del__
    self.close()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/sslproto.py", line 316, in close
    self._ssl_protocol._start_shutdown()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/sslproto.py", line 590, in _start_shutdown
    self._abort()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/sslproto.py", line 731, in _abort
    self._transport.abort()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/selector_events.py", line 680, in abort
    self._force_close(None)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/selector_events.py", line 731, in _force_close
    self._loop.call_soon(self._call_connection_lost, exc)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/base_events.py", line 746, in call_soon
    self._check_closed()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-aarch64/build/Lib/asyncio/base_events.py", line 510, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

mdickinson · 2020-10-01T16:56:42Z

It seems rather unlikely that that buildbot failure is related (test_asyncio changed the environment). But it's odd that I don't see the same buildbot failure on other recent PRs.

rhettinger · 2020-10-04T05:38:58Z

Why was this merged? Guido was clear, "It should not go on the numeric tower."

…d library numeric types (GH-6121)" This reverts commit 58a7da9.

…d library numeric types (GH-6121)" (GH-22584) This reverts commit 58a7da9.

* origin/master: (147 commits) Fix the attribute names in the docstring of GenericAlias (pythonGH-22594) bpo-39337: Add a test case for normalizing of codec names (pythonGH-19069) bpo-41557: Update Windows installer to use SQLite 3.33.0 (pythonGH-21960) bpo-41976: Fix the fallback to gcc of ctypes.util.find_library when using gcc>9 (pythonGH-22598) bpo-41306: Allow scale value to not be rounded (pythonGH-21715) bpo-41970: Avoid test failure in test_lib2to3 if the module is already imported (pythonGH-22595) bpo-41376: Fix the documentation of `site.getusersitepackages()` (pythonGH-21602) Revert "bpo-26680: Incorporate is_integer in all built-in and standard library numeric types (pythonGH-6121)" (pythonGH-22584) bpo-41923: PEP 613: Add TypeAlias to typing module (python#22532) Fix comment about PyObject_IsTrue. (pythonGH-22343) bpo-38605: Make 'from __future__ import annotations' the default (pythonGH-20434) bpo-41905: Add abc.update_abstractmethods() (pythonGH-22485) bpo-41944: No longer call eval() on content received via HTTP in the UnicodeNames tests (pythonGH-22575) bpo-41944: No longer call eval() on content received via HTTP in the CJK codec tests (pythonGH-22566) Post 3.10.0a1 Python 3.10.0a1 bpo-41584: clarify when the reflected method of a binary arithemtic operator is called (python#22505) bpo-41939: Fix test_site.test_license_exists_at_url() (python#22559) bpo-41774: Tweak new programming FAQ entry (pythonGH-22562) bpo-41936. Remove macros Py_ALLOW_RECURSION/Py_END_ALLOW_RECURSION (pythonGH-22552) ...

…y numeric types (pythonGH-6121) * bpo-26680: Adds support for int.is_integer() for compatibility with float.is_integer(). The int.is_integer() method always returns True. * bpo-26680: Adds a test to ensure that False.is_integer() and True.is_integer() are always True. * bpo-26680: Adds Real.is_integer() with a trivial implementation using conversion to int. This default implementation is intended to reduce the workload for subclass implementers. It is not robust in the presence of infinities or NaNs and may have suboptimal performance for other types. * bpo-26680: Adds Rational.is_integer which returns True if the denominator is one. This implementation assumes the Rational is represented in it's lowest form, as required by the class docstring. * bpo-26680: Adds Integral.is_integer which always returns True. * bpo-26680: Adds tests for Fraction.is_integer called as an instance method. The tests for the Rational abstract base class use an unbound method to sidestep the inability to directly instantiate Rational. These tests check that everything works correct as an instance method. * bpo-26680: Updates documentation for Real.is_integer and built-ins int and float. The call x.is_integer() is now listed in the table of operations which apply to all numeric types except complex, with a reference to the full documentation for Real.is_integer(). Mention of is_integer() has been removed from the section 'Additional Methods on Float'. The documentation for Real.is_integer() describes its purpose, and mentions that it should be overridden for performance reasons, or to handle special values like NaN. * bpo-26680: Adds Decimal.is_integer to the Python and C implementations. The C implementation of Decimal already implements and uses mpd_isinteger internally, we just expose the existing function to Python. The Python implementation uses internal conversion to integer using to_integral_value(). In both cases, the corresponding context methods are also implemented. Tests and documentation are included. * bpo-26680: Updates the ACKS file. * bpo-26680: NEWS entries for int, the numeric ABCs and Decimal. Co-authored-by: Robert Smallshire <rob@sixty-north.com>

…d library numeric types (pythonGH-6121)" (pythonGH-22584) This reverts commit 58a7da9.

Match `int.is_integer`, which was added in python/cpython#6121

rob-smallshire requested review from rhettinger and skrah as code owners March 15, 2018 10:39

the-knights-who-say-ni added the CLA signed label Mar 15, 2018

bedevere-bot added the awaiting review label Mar 15, 2018

mdickinson suggested changes May 12, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 12, 2019

rob-smallshire force-pushed the is_integer branch from e745028 to e6a66a7 Compare September 17, 2020 11:54

rob-smallshire force-pushed the is_integer branch from e6a66a7 to 1cacea7 Compare September 17, 2020 12:03

rob-smallshire added 2 commits September 18, 2020 10:55

bpo-26680: Adds support for int.is_integer() for compatibility with f…

d80b017

…loat.is_integer(). The int.is_integer() method always returns True.

bpo-26680: Adds a test to ensure that False.is_integer() and True.is_…

b46a9dc

…integer() are always True.

rob-smallshire force-pushed the is_integer branch from 1cacea7 to 138f704 Compare September 18, 2020 09:16

eric-wieser reviewed Sep 18, 2020

View reviewed changes

rob-smallshire force-pushed the is_integer branch from 138f704 to a6a5490 Compare September 18, 2020 10:03

rob-smallshire added 3 commits September 18, 2020 12:36

bpo-26680: Adds Rational.is_integer which returns True if the denomin…

f78bf5c

…ator is one. This implementation assumes the Rational is represented in it's lowest form, as required by the class docstring.

bpo-26680: Adds Integral.is_integer which always returns True.

6a905a8

rob-smallshire force-pushed the is_integer branch from 1ded358 to 821aad6 Compare September 18, 2020 13:00

rob-smallshire force-pushed the is_integer branch from 821aad6 to fba90b3 Compare September 18, 2020 14:32

rob-smallshire added 3 commits September 18, 2020 16:51

bpo-26680: Updates the ACKS file.

a9e364d

bpo-26680: NEWS entries for int, the numeric ABCs and Decimal.

11db7f8

rob-smallshire force-pushed the is_integer branch from fba90b3 to 11db7f8 Compare September 18, 2020 14:52

mdickinson approved these changes Sep 19, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 19, 2020

mdickinson merged commit 58a7da9 into python:master Oct 1, 2020

bedevere-bot removed the awaiting merge label Oct 1, 2020

eric-wieser mentioned this pull request Oct 1, 2020

Add methods from the builtin float types to the numpy floating point types numpy/numpy#13375

Open

5 tasks

rhettinger added a commit that referenced this pull request Oct 7, 2020

Revert "bpo-26680: Incorporate is_integer in all built-in and standar…

1cc1a8f

…d library numeric types (GH-6121)" This reverts commit 58a7da9.

rhettinger mentioned this pull request Oct 7, 2020

Revert "bpo-26680: Incorporate is_integer in all built-in and standard library numeric types" #22584

Merged

rhettinger added a commit that referenced this pull request Oct 7, 2020

Revert "bpo-26680: Incorporate is_integer in all built-in and standar…

4e0ce82

…d library numeric types (GH-6121)" (GH-22584) This reverts commit 58a7da9.

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020

Revert "bpo-26680: Incorporate is_integer in all built-in and standar…

9237628

…d library numeric types (pythonGH-6121)" (pythonGH-22584) This reverts commit 58a7da9.

BvB93 pushed a commit to BvB93/numpy that referenced this pull request Aug 31, 2021

ENH: Add integer.is_integer

9f11564

Match `int.is_integer`, which was added in python/cpython#6121

howjmay pushed a commit to howjmay/numpy that referenced this pull request Sep 29, 2021

ENH: Add integer.is_integer

a8ca01c

Match `int.is_integer`, which was added in python/cpython#6121

mdickinson mentioned this pull request Dec 23, 2022

Improve duck type compatibility of int with float #100268

Closed


		Return :const:`True` if the argument is a finite integral value and
		:const:`False` otherwise.

	return self.denominator == 1
	return self.numerator % self.denominator == 0

bpo-26680: Incorporate is_integer in all built-in and standard library numeric types #6121

bpo-26680: Incorporate is_integer in all built-in and standard library numeric types #6121

Conversation

rob-smallshire commented Mar 15, 2018 • edited Loading

skrah commented Mar 26, 2019

rob-smallshire commented Mar 26, 2019

mdickinson commented Mar 26, 2019

rob-smallshire commented Apr 7, 2019

skrah commented Apr 7, 2019

mdickinson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson May 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 12, 2019

rob-smallshire commented Aug 22, 2019

csabella commented May 22, 2020

rob-smallshire commented May 24, 2020 • edited Loading

eric-wieser commented Aug 20, 2020

rob-smallshire commented Aug 20, 2020

rob-smallshire commented Sep 17, 2020

mdickinson commented Sep 18, 2020

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

rob-smallshire commented Sep 18, 2020

mdickinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Sep 19, 2020

rob-smallshire commented Oct 1, 2020

mdickinson commented Oct 1, 2020

bedevere-bot commented Oct 1, 2020

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

mdickinson commented Oct 1, 2020

rhettinger commented Oct 4, 2020

rob-smallshire commented Mar 15, 2018 •

edited

Loading

mdickinson May 12, 2019 •

edited

Loading

rob-smallshire commented May 24, 2020 •

edited

Loading