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

cmath is numerically unsound #45722

Closed
inducer mannequin opened this issue Nov 3, 2007 · 23 comments
Closed

cmath is numerically unsound #45722

inducer mannequin opened this issue Nov 3, 2007 · 23 comments
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@inducer
Copy link
Mannequin

inducer mannequin commented Nov 3, 2007

BPO 1381
Nosy @gvanrossum, @loewis, @jcea, @mdickinson, @tiran
Files
  • cmath_py.py
  • cmath.patch
  • cmath3.patch: Updated patch for cmath module, with docs and tests
  • 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 2008-04-23.17:08:48.543>
    created_at = <Date 2007-11-03.18:58:58.226>
    labels = ['extension-modules', 'type-bug']
    title = 'cmath is numerically unsound'
    updated_at = <Date 2010-05-11.15:01:04.386>
    user = 'https://bugs.python.org/inducer'

    bugs.python.org fields:

    activity = <Date 2010-05-11.15:01:04.386>
    actor = 'bins'
    assignee = 'none'
    closed = True
    closed_date = <Date 2008-04-23.17:08:48.543>
    closer = 'mark.dickinson'
    components = ['Extension Modules']
    creation = <Date 2007-11-03.18:58:58.226>
    creator = 'inducer'
    dependencies = []
    files = ['8685', '8807', '9065']
    hgrepos = []
    issue_num = 1381
    keywords = ['patch']
    message_count = 23.0
    messages = ['57088', '57089', '57090', '57092', '57098', '57104', '57835', '59268', '59557', '59568', '59570', '59572', '59637', '59638', '61499', '65703', '105510', '105512', '105515', '105517', '105519', '105521', '105522']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'loewis', 'jcea', 'mark.dickinson', 'alanmcintyre', 'inducer', 'christian.heimes', 'bins']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue1381'
    versions = ['Python 2.6', 'Python 3.0', 'Python 3.1']

    @inducer
    Copy link
    Mannequin Author

    inducer mannequin commented Nov 3, 2007

    This here basically says it all:

    >>> import cmath;[cmath.asinh(i*1e-17).real for i in range(0,20)]
    [4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16, 4.4408920985006257e-16,
    4.4408920985006257e-16, 4.4408920985006257e-16]

    The boost.math toolkit at [2] is an implementation that does better in
    the above (real-only) aspect.
    [2] http://freespace.virgin.net/boost.regex/toolkit/html/index.html

    Tim Peters remarks in [1] that basically all of cmath is unsound.
    http://mail.python.org/pipermail/python-bugs-list/2001-February/004126.html

    I just wanted to make sure that this issue remains on the radar.

    @inducer inducer mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Nov 3, 2007
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Nov 3, 2007

    Can you propose a patch?

    @inducer
    Copy link
    Mannequin Author

    inducer mannequin commented Nov 3, 2007

    On Samstag 03 November 2007, Martin v. Löwis wrote:

    Martin v. Löwis added the comment:

    Can you propose a patch?

    Other than point at how boost.math does things, I don't have the time to work
    on this right now, sorry.

    Andreas

    @alanmcintyre
    Copy link
    Mannequin

    alanmcintyre mannequin commented Nov 4, 2007

    I have to review a few complex math topics for some of my current course
    work, so I wouldn't mind taking a look into this. I can't promise I'll
    have the time required to make all of cmath correct (assuming it's as
    unsound as claimed), but I'll do what I can.

    @mdickinson
    Copy link
    Member

    I took a look at this a while back, and got as far as writing a pure
    Python drop-in replacement for cmath, based on Kahan's "branch cuts for
    elementary functions" paper. This fixes a variety of problems in cmath,
    including the buggy branch cuts for asinh. File attached, in case it's
    of any use.

    As Tim Peters pointed out, the real problem here is a lack of decent
    unit tests for these functions. I have tests for the file above, but
    they assume IEEE754 floats, which is probably not an acceptable
    assumption in general.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Nov 4, 2007

    It would be ok if a test is only run on a system with IEEE floats, and
    skipped elsewhere. For all practical purposes, Python assumes that all
    systems have IEEE float.

    @mdickinson
    Copy link
    Member

    Here is (quite a large) patch, cmath.patch, that fixes a variety of
    problems in the current cmath module. A summary of the changes:

    • sqrt, log, acos, acosh, asin, asinh, atan, atanh no longer produce
      overflow errors for very large inputs

    • exp, cos, sin, tan, cosh, sinh, tanh produce valid answers in some cases
      where they incorrectly overflowed before.

    • sqrt and log are more accurate for tiny numbers

    • numeric problems in some functions (especially asinh and asin) should
      have been fixed

    • the branch cuts for asinh have been moved to the standard positions

    • the direction of continuity for the branch cuts of tan, tanh have been
      fixed

    • on systems with full hardware and system support for signed zeros (most
      modern systems), functions with a branch cut are continuous on both sides
      of the branch cut. (As recommended by the C99 standard, amongst others.)

    The patch includes documentation updates, but there are still no tests.
    (My current tests make heavy use of the MPFR library, and assume IEEE-754
    format doubles, so need to be seriously reworked.) The tests are on my to-
    do list, but I'm unlikely to find time for them before the new year. In
    the meantime, I'd very much appreciate any feedback on this patch, if
    anyone has the time/energy/inclination to look at it. (Andreas: are you
    in a position to give this a test-run?)

    @mdickinson
    Copy link
    Member

    Here's an updated patch for cmath, complete with tests and documentation changes.
    There's an extra file, Lib/test/cmath.ctest, containing test values for the various functions;
    these test values were obtained using MPFR and interval arithmetic, and (modulo bugs) should
    all be correctly rounded.

    Any feedback would be much appreciated.

    @mdickinson
    Copy link
    Member

    A note to any potential reviewers for this patch (cmath3.patch): I'm especially
    looking for non-mathematical feedback here, so please don't be put off by the
    mathematics. Some questions:

    • there's a new file cmath.ctest containing testcases; should this be put
      directly into Lib/test (where it is right now), or is there a better place. Is
      the name of this file reasonable?

    • is the new stuff in pyport.h (checks for _copysign and copysign, and automatic
      renaming of _copysign to copysign) okay? Should it be in cmathmodule.c instead?

    • is the code in test_cmath that looks for the testcase file okay?

    And it would be really great if someone with access to a Windows box could just
    verify that the tests pass with this patch applied; I've only tested it on OS X
    and Linux.

    @tiran
    Copy link
    Member

    tiran commented Jan 8, 2008

    • I've added a configure test for copysign a while ago. I'm using this
      constructor for Windows compatibility in mathmodule.c:
    #ifdef MS_WINDOWS
    #  define copysign _copysign
    #  define HAVE_COPYSIGN 1
    #endif
    #ifdef HAVE_COPYSIGN
    #endif

    I know no platform besides Windows with a _copysign method. You don't
    have to add tests for _copysign for Windows. You may want to add the
    code to PC/pyconfig.h and define HAVE_COPYSIGN there.

    • test_file = testdir + os.sep + 'cmath.ctest
      make that:
      test_file = os.path.join(testdir, 'cmath.ctest')

    • Windows doesn't have log1p, the hyperbolic area functions (asinh) and
      some other functions, too. IIRC Windows does only implement the Bessel
      functions of 1st and 2nd kind but non of the other C99 math functions.

    @mdickinson
    Copy link
    Member

    Okay: would it be okay for me to move the #ifdef MS_WINDOWS sequence
    somewhere where it can be used by both mathmodule.c and cmathmodule.c,
    then?

    There are replacements for log1p and asinh in the patch, so it shouldn't
    matter than they're missing on Windows---it might mean that the accuracy
    of some of the functions is slightly reduced on Windows, though.

    @tiran
    Copy link
    Member

    tiran commented Jan 8, 2008

    It may even be a good idea to move asinh, copysign and log1p to a new
    file Python/pymath.c and add compensatory implementations of acosh,
    atanh and expm1, too. The math related code in pyport.h could then be
    moved to Include/pymath.h. In the past some people have asked me to
    implement the reverse hyperbolic functions for the math module.

    This is beyond a simple fix for the cmath module and should be discussed
    on the python-dev list first.

    @tiran
    Copy link
    Member

    tiran commented Jan 10, 2008

    Guido, how do you like the idea of Include/pymath.h and Python/pymath.c
    containing supplementary mathematical functions and mathematical
    constants? Mark's patch for cmath is rather large, can it still be
    applied to 2.5?

    @gvanrossum
    Copy link
    Member

    Are you crazy? Adding new features to 2.5? No way!

    @tiran
    Copy link
    Member

    tiran commented Jan 22, 2008

    See bpo-1640 and svn+ssh://pythondev@svn.python.org/python/branches/trunk-math

    @tiran tiran added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels Jan 22, 2008
    @mdickinson
    Copy link
    Member

    Substantial fixes for the cmath module went into the trunk and the py3k
    branches as part of the merge of the trunk-math branch. These fixes
    address the asinh problems in this issue, amongst other things. See
    r62380 (trunk) and r62384 (py3k).

    @bins
    Copy link
    Mannequin

    bins mannequin commented May 11, 2010

    hi there,

    it seems there is still a problem, at least with asinh(-2j)

    see:

    $ python
    Python 2.6.5 (r265:79063, Apr  1 2010, 05:28:39) 
    [GCC 4.4.3 20100316 (prerelease)] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import numpy as np
    >>> np.__version__
    '1.4.0'
    >>> import cmath
    >>> cmath.asinh(-2j)
     (1.3169578969248166-1.5707963267948966j)
    
    >>> np.arcsinh(-2j)
    (-1.3169578969248164-1.5707963267948966j)

    same behaviour for python3.1:
    $ python3
    Python 3.1.2 (r312:79147, Apr 1 2010, 09:12:21)
    [GCC 4.4.3 20100316 (prerelease)] on linux2
    Type "help", "copyright", "credits" or "license" for more information.

    >>> import cmath
    >>> cmath.asinh(-2j)
    (1.3169578969248166-1.5707963267948966j)

    cheers,
    sebastien.

    @mdickinson
    Copy link
    Member

    Python's result looks fine to me, as does numpy's: they're both giving a valid inverse hyperbolic sine:

    >>> from cmath import sinh
    >>> sinh(1.3169578969248166-1.5707963267948966j)  
    (1.0605752387249067e-16-1.9999999999999998j)
    >>> sinh(-1.3169578969248164-1.5707963267948966j)
    (-1.0605752387249064e-16-1.9999999999999993j)

    Perhaps numpy is using different branch cuts?

    @mdickinson
    Copy link
    Member

    A bit more explanation: Python takes account of the sign of zero when deciding which side of the branch cut a value lies, which is the proper thing to do when you have signed zeros available (according to the likes of Kahan, anyway); I suspect that numpy isn't doing that, but is treating all values that lie directly on a branch in the same way.

    In this case there's a branch cut from -1j down to -1j*inf. Values just to the right of that branch cut (i.e., with positive real part) should have a result with positive real part; values just to the left of it should have negative real part:

    Some results (using complex() to create the values, since other ways of creating complex numbers are prone to changing the sign of a zero):

    >>> asinh(complex(0.0, -2.0))
    (1.3169578969248166-1.5707963267948966j)
    >>> asinh(complex(1e-10, -2.0))
    (1.3169578969248166-1.5707963267371616j)
    >>> asinh(complex(-0.0, -2.0))
    (-1.3169578969248166-1.5707963267948966j)
    >>> asinh(complex(-1e-10, -2.0))
    (-1.3169578969248166-1.5707963267371616j)

    So the cmath module is working as intended here. numpy may or may not be working as intended: I don't know how much they care about branch cut continuity.

    @bins
    Copy link
    Mannequin

    bins mannequin commented May 11, 2010

    hi Mark,

    that may very well be so, but I'd naively standardize on C/Fortran behaviour (but that's probably my physicist bias)

    on my platform, the following piece of C-code:

    $ cat test_cmath.c
    #include <complex.h>
    #include <stdio.h>
    
    int main(int argc, char** argv)
    {
      complex c = casinh(-2*I);
      printf("asinh(-2j) = %g + %gi\n", creal(c), cimag(c));
      return 0;
    }
    /* EOF */

    gives:
    $ ./a.out
    asinh(-2j) = -1.31696 + -1.5708i

    cheers,
    sebastien.

    @mdickinson
    Copy link
    Member

    that may very well be so, but I'd naively standardize on C/Fortran > behaviour (but that's probably my physicist bias)

    Yep, that's exactly what Python does. :) (Also follows the LISP standard).

    Note that in your program, you're feeding complex(-0.0, -2.0) to asinh,
    not complex(0.0, 2.0).

    @mdickinson
    Copy link
    Member

    Note that in your program, you're feeding complex(-0.0, -2.0) to asinh,
    not complex(0.0, 2.0).

    Bah; that should be complex(0.0, -2.0) in the second line, of course.

    Anyway, try passing conj(2*I) to asinh in your C program and see what happens. :)

    @bins
    Copy link
    Mannequin

    bins mannequin commented May 11, 2010

    Note that in your program, you're feeding complex(-0.0, -2.0) to asinh,
    not complex(0.0, -2.0).

    ah!
    (ducking)

    @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
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants