| msg243914 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
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) * |
Date: 2015-05-24 08:45 |
| I'm now working this into a patch against current default. |
|
|
| msg243976 - (view) |
Author: Tal Einat (taleinat) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 "", line 1, in OverflowError: int too large to convert to float >>> math.isclose(10**999, 10**400000) Traceback (most recent call last): File "", line 1, in OverflowError: int too large to convert to float |
|
|
| msg244375 - (view) |
Author: Stefan Krah (skrah) * |
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) * |
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) * |
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) * |
Date: 2015-06-02 23:17 |
| Can this issue be closed now? |
|
|
| msg244728 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2015-06-03 06:08 |
| Indeed, it should be. |
|
|