classification
Title: PEP 485 (math.isclose) implementation
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Chris.Barker, mark.dickinson, ncoghlan, paul.moore, python-dev, rhettinger, scoder, skrah, stutzbach, taleinat, yselivanov
Priority: normal Keywords: patch

Created on 2015-05-23 13:18 by ncoghlan, last changed 2015-06-03 08:39 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
math_isclose.patch taleinat, 2015-05-24 11:05 initial patch review
math_isclose_v2.patch taleinat, 2015-05-25 19:54 revised patch with minor documentation improvements review
math_isclose_v3.patch taleinat, 2015-05-26 20:28 revised patch with keyword-only parameters and refactored tests
math_isclose_v4.patch taleinat, 2015-05-27 15:09 revised patch with fixed tests and reworded doc-string
isclose.patch taleinat, 2015-05-28 09:25 complete patch including complex version of isclose()
Messages (29)
msg243914 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 13:18
Tracking issue for the PEP 485 math.isclose() implementation: https://www.python.org/dev/peps/pep-0485/

Chris's implementation review request to python-dev: https://mail.python.org/pipermail/python-dev/2015-May/140031.html

Working repo: https://github.com/PythonCHB/close_pep
msg243972 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-24 08:45
I'm now working this into a patch against current default.
msg243976 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-24 11:05
Attached is a patch based on Chris Barker's implementation on github[1]. This includes only the C implementation, as well as tests, documentation and entries in NEWS and whatsnew.

I had to keep the (PyCFunction) cast in the module method list in Modules/mathmodule.c. That part should certainly be reviewed.

As for documentation etc., I took the best wording I could find from the PEP and the docs in the github repo, and made very slight changes only where I though they made things significantly clearer.

.. [1]: https://github.com/PythonCHB/close_pep
msg243979 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-24 12:41
The cast is correct and required (the actual signature is determined by the METH_* flags).

Patch LGTM, FWIW.
msg243984 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-24 13:42
I have a question regarding complex values. The code (from Chris Barker) doesn't support complex values (only things that can be converted into doubles). However, the PEP states the following under "Non-float types":

"complex : for complex, the absolute value of the complex values will be used for scaling and comparison. If a complex tolerance is passed in, the absolute value will be used as the tolerance."

Should math.isclose() support complex values? Should an equivalent function be added to cmath? Should we just leave things as they are and remove mention of complex values from the PEP (it isn't mentioned in the docs)?
msg243985 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-24 13:45
Eventually, I think a corresponding function should be added to cmath. math.isclose() shouldn't deal with complex values by itself (other than rejecting them as non-floatish input).
msg244049 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-25 19:54
Attached is a slightly revised patch.

This mostly fixes minor documentation wording and formatting issues, including those pointed out by Chris Barker on the python-dev mailing list.

Also, since it has been decided to support complex values only in a separate cmath.isclose() function, the previously included commented-out complex test cases have been removed.
msg244103 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-26 13:44
Significant questions brought up by Berker Peksağ in his review of the latest patch (thanks for the review!):

1. Should the tolerance parameters be keyword-only? Berker suggests that they should be. I agree.

2. Should the math.isclose() tests be split into a separate TestCase class with many separate methods? It is currently a single method which does all of the testing for math.isclose(). (Chris's original code has it separated into several TestCase classes; I consolidated it into a single method to keep in line with the current structure of the math module's tests.)
msg244104 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-26 13:56
Regarding the tests, I now realize that most of them should be reused for testing cmath.isclose(), which means they'll have to be defined outside of test_math.MathTests.
msg244133 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-26 20:28
Attached is a revised patch including changed due to the reviews by Berker and Yuri.

The significant changes from the previous patch are:

1. The "rel_tol" and "abs_tol" parameters have been made keyword-only.

2. The tests have been extracted into a separate TestCase sub-class. They are now better organized and will be easy to re-use for testing cmath.isclose when it is added (hopefully soon).
msg244173 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-27 15:09
Attached yet another revised version of the math.isclose() patch.

This patch fixes a problem with the tests in the previous patch which causes them to fail when the full test suite is run.

I've also slightly reworded the doc-string.

Hopefully this is ready to go in!
msg244291 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-28 09:25
Hopefully final patch attached. This adds cmath.isclose() along with relevant tests and documentation.

Note that cmath.isclose() rejects complex tolerances -- only the values may be complex.
msg244292 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-28 11:45
I think users may be surprised that any two large Decimals like
"1e400000" and "1e999" are "close".  In the Decimal world these
aren't infinite.
msg244293 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-28 12:05
@Stefan: Well, this seems to already be the situation with the rest of the math module:

>>> math.isinf(Decimal("1e999"))
True

>>> math.sqrt(Decimal("1e999"))
inf

Properly handling other types which are convertible to floats, such as Decimal and Fraction, is outside the scope of this issue.
msg244294 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-28 12:07
> Properly handling other types which are convertible to floats, such as Decimal and Fraction, is outside the scope of this issue.

... and outside of the scope of the math module in general. It's inherently
floating point based.
msg244295 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-28 12:14
Alright then, but is anyone going to review this so that it can go in? The actual code to review is very short, the documentation is clearly written and not too long, and the tests are easy to read!
msg244296 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-05-28 12:49
Looks OK to me.

I assume the differences between the math and cmath code and tests is because cmath uses Argument Clinic and math doesn't, and cmath uses unittest.main whereas math adds the suites manually? As far as I can see, that's what's going on.
msg244297 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-28 12:56
Indeed, those are major reasons for differences.

I avoided using Argument Clinic for math.isclose() because there is a pending conversion patch for the entire math module and I didn't want to cause unnecessary merge conflicts.

Is Paul's okay enough for me to commit this, or should we also get an okay from one of the three people listed next to the math module on the experts index?
msg244308 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-28 15:46
> It's inherently floating point based.

Except for floor() and ceil() though.  The wording in the PEP
under "non-float" types made me think that something similar
was intended here.

Personally I'm fine with math being float-only.
msg244311 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-28 15:53
Also, I must say that returning inf in sqrt() bothers me much less
than the assertion that two numbers with a gigantic relative error
have a relerr of 1e-9.
msg244338 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-28 18:52
@Stefan K.: I tend to agree, but still think that's a separate issue. math.isclose() certainly shouldn't be checking the type of its arguments.


While we're on the subject, though, trying to convert a very large int to float fails with an OverflowError. Perhaps Decimal should do the same?

>>> float(10**999)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: int too large to convert to float

>>> math.isclose(10**999, 10**400000)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: int too large to convert to float
msg244375 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-05-29 13:33
> While we're on the subject, though, trying to convert a very large int to float fails with an OverflowError. Perhaps Decimal should do the same?

I've always viewed float() as a cast. For casting Decimal's behavior
seems to be the right one to me.

If we go ahead with implicit casts for isclose(), I still think
the PEP's non-float section should be changed:  It sounds as if
native support was planned for Decimal.

Does someone have the tracker id of Chris?
msg244416 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-29 20:51
It's Chris.Barker. I've added him to the nosy list.
msg244427 - (view) Author: Christopher Barker (Chris.Barker) Date: 2015-05-29 22:33
Sorry for the confusion:

when I wrote the PEP, I was thinking in terms of a Python, duck-typed implementation.

Now that it's in C, that doesn't work so well.

I will update the PEP to indicate that it is float-only, or complex for the cmath implementation (thanks, Tal!).

Any other type will be converted to float if possible, with the limitations that that has.

As for Decimal -- the "right" thing to do would be to do a native Decimal implementation -- but that would live in the decimal module, maybe as a method on the Decimal type. (or stand alone -- not sure)

Fraction doesn't have the same precision issues as floats, so not sure what the point is.
msg244556 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-05-31 19:23
I've just committed this into 3.5 and 3.6.

(I accidentally included the wrong issue number in the commit message, so the bot hasn't posted here about it. Sorry!)
msg244557 - (view) Author: Christopher Barker (Chris.Barker) Date: 2015-05-31 19:55
I wrote:
"""I will update the PEP to indicate that it is float-only, or complex for the cmath implementation (thanks, Tal!)."""

Done:

https://github.com/PythonCHB/close_pep/blob/master/pep-0485.txt

Hopefully pushed to the official PEP repo soon.
msg244571 - (view) Author: Roundup Robot (python-dev) Date: 2015-06-01 06:48
New changeset bbb3a3129c12 by Serhiy Storchaka in branch '3.5':
Moved Misc/NEWS entry (issue #24270) to correct section.
https://hg.python.org/cpython/rev/bbb3a3129c12

New changeset ff1938d12240 by Serhiy Storchaka in branch 'default':
Moved Misc/NEWS entry (issue #24270) to correct section.
https://hg.python.org/cpython/rev/ff1938d12240
msg244712 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-02 23:17
Can this issue be closed now?
msg244728 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2015-06-03 06:08
Indeed, it should be.
History
Date User Action Args
2015-06-03 08:39:22berker.peksagsetstatus: open -> closed
resolution: fixed
stage: resolved
2015-06-03 06:08:07taleinatsetmessages: + msg244728
2015-06-02 23:17:51yselivanovsetnosy: + yselivanov
messages: + msg244712
2015-06-01 06:48:06python-devsetnosy: + python-dev
messages: + msg244571
2015-05-31 19:55:54Chris.Barkersetmessages: + msg244557
2015-05-31 19:23:18taleinatsetmessages: + msg244556
2015-05-29 22:33:05Chris.Barkersetmessages: + msg244427
2015-05-29 20:51:15taleinatsetnosy: + Chris.Barker
messages: + msg244416
2015-05-29 13:33:04skrahsetmessages: + msg244375
2015-05-28 18:52:09taleinatsetmessages: + msg244338
2015-05-28 15:53:22skrahsetmessages: + msg244311
2015-05-28 15:46:08skrahsetmessages: + msg244308
2015-05-28 12:56:37taleinatsetmessages: + msg244297
2015-05-28 12:49:13paul.mooresetnosy: + paul.moore
messages: + msg244296
2015-05-28 12:14:55taleinatsetmessages: + msg244295
2015-05-28 12:07:44scodersetmessages: + msg244294
2015-05-28 12:05:21taleinatsetmessages: + msg244293
2015-05-28 11:45:14skrahsetnosy: + skrah
messages: + msg244292
2015-05-28 09:25:04taleinatsetfiles: + isclose.patch

messages: + msg244291
2015-05-27 15:09:32taleinatsetfiles: + math_isclose_v4.patch

messages: + msg244173
2015-05-26 20:28:30taleinatsetfiles: + math_isclose_v3.patch

messages: + msg244133
2015-05-26 13:56:49taleinatsetmessages: + msg244104
2015-05-26 13:44:56taleinatsetmessages: + msg244103
2015-05-25 19:54:11taleinatsetfiles: + math_isclose_v2.patch

messages: + msg244049
2015-05-24 13:49:05taleinatsetnosy: + rhettinger, mark.dickinson, stutzbach
2015-05-24 13:45:37scodersetmessages: + msg243985
2015-05-24 13:42:34taleinatsetmessages: + msg243984
2015-05-24 12:41:57scodersetnosy: + scoder
messages: + msg243979

components: + Library (Lib)
type: enhancement
2015-05-24 11:05:22taleinatsetfiles: + math_isclose.patch
keywords: + patch
messages: + msg243976
2015-05-24 08:45:30taleinatsetnosy: + taleinat
messages: + msg243972
2015-05-23 13:18:03ncoghlancreate