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

PEP 485 (math.isclose) implementation #68458

Closed
ncoghlan opened this issue May 23, 2015 · 29 comments
Closed

PEP 485 (math.isclose) implementation #68458

ncoghlan opened this issue May 23, 2015 · 29 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented May 23, 2015

BPO 24270
Nosy @rhettinger, @pfmoore, @mdickinson, @ncoghlan, @scoder, @taleinat, @skrah, @1st1
Files
  • math_isclose.patch: initial patch
  • math_isclose_v2.patch: revised patch with minor documentation improvements
  • math_isclose_v3.patch: revised patch with keyword-only parameters and refactored tests
  • math_isclose_v4.patch: revised patch with fixed tests and reworded doc-string
  • isclose.patch: complete patch including complex version of isclose()
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-06-03.08:39:22.417>
    created_at = <Date 2015-05-23.13:18:03.154>
    labels = ['type-feature', 'library']
    title = 'PEP 485 (math.isclose) implementation'
    updated_at = <Date 2015-06-03.08:39:22.416>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2015-06-03.08:39:22.416>
    actor = 'berker.peksag'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-06-03.08:39:22.417>
    closer = 'berker.peksag'
    components = ['Library (Lib)']
    creation = <Date 2015-05-23.13:18:03.154>
    creator = 'ncoghlan'
    dependencies = []
    files = ['39481', '39495', '39509', '39518', '39532']
    hgrepos = []
    issue_num = 24270
    keywords = ['patch']
    message_count = 29.0
    messages = ['243914', '243972', '243976', '243979', '243984', '243985', '244049', '244103', '244104', '244133', '244173', '244291', '244292', '244293', '244294', '244295', '244296', '244297', '244308', '244311', '244338', '244375', '244416', '244427', '244556', '244557', '244571', '244712', '244728']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'paul.moore', 'mark.dickinson', 'ncoghlan', 'scoder', 'taleinat', 'stutzbach', 'Chris.Barker', 'skrah', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24270'
    versions = ['Python 3.5']

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented May 23, 2015

    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

    @taleinat
    Copy link
    Contributor

    taleinat commented May 24, 2015

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

    @taleinat
    Copy link
    Contributor

    taleinat commented May 24, 2015

    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

    @scoder
    Copy link
    Contributor

    scoder commented May 24, 2015

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

    Patch LGTM, FWIW.

    @scoder scoder added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 24, 2015
    @taleinat
    Copy link
    Contributor

    taleinat commented May 24, 2015

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

    @scoder
    Copy link
    Contributor

    scoder commented May 24, 2015

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

    @taleinat
    Copy link
    Contributor

    taleinat commented May 25, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 26, 2015

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

    @taleinat
    Copy link
    Contributor

    taleinat commented May 26, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 26, 2015

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

    @taleinat
    Copy link
    Contributor

    taleinat commented May 27, 2015

    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!

    @taleinat
    Copy link
    Contributor

    taleinat commented May 28, 2015

    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.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 28, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 28, 2015

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

    @scoder
    Copy link
    Contributor

    scoder commented May 28, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 28, 2015

    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!

    @pfmoore
    Copy link
    Member

    pfmoore commented May 28, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 28, 2015

    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?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 28, 2015

    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.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 28, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 28, 2015

    @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

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented May 29, 2015

    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?

    @taleinat
    Copy link
    Contributor

    taleinat commented May 29, 2015

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

    @ChrisBarker
    Copy link
    Mannequin

    ChrisBarker mannequin commented May 29, 2015

    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.

    @taleinat
    Copy link
    Contributor

    taleinat commented May 31, 2015

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

    @ChrisBarker
    Copy link
    Mannequin

    ChrisBarker mannequin commented May 31, 2015

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 1, 2015

    New changeset bbb3a3129c12 by Serhiy Storchaka in branch '3.5':
    Moved Misc/NEWS entry (issue bpo-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 bpo-24270) to correct section.
    https://hg.python.org/cpython/rev/ff1938d12240

    @1st1
    Copy link
    Member

    1st1 commented Jun 2, 2015

    Can this issue be closed now?

    @taleinat
    Copy link
    Contributor

    taleinat commented Jun 3, 2015

    Indeed, it should be.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants