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

I'm now working this into a patch against current default.

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

The cast is correct and required (the actual signature is determined by the METH_* flags).

Patch LGTM, FWIW.

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

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

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.

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

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.

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

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!

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.

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.

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

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

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!

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.

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?

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

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.

@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

> 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?

It's Chris.Barker. I've added him to the nosy list.

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.

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

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.

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

Can this issue be closed now?

Indeed, it should be.