Closed tiran closed 16 years ago
One more comment: the Py_IS_NAN test in acosh is likely never reached, since the comparison x >= two_pow_p28 will probably evaluate to 0 when x is a NaN.
Also: -1.0 shouldn't be returned at this level to indicate an error; these are pure C functions we're writing---the Python wrappers, and Python error conventions, apply further up the chain somewhere.
Just so I'm doing something a little more constructive than yelling criticisms from the sideline,
I've attached a file invhyp.c that's how I think the C functions should look. Some notes:
it doesn't touch errno, but lets the platform decide how to handle errors (i.e. produce a special value/set errno/signal a floating-point exception/some combination of these). This will make the asinh, acosh, atanh functions behave in the same way that the regular libm functions behave on any platform. So e.g. if a particular platform is used to setting errno for domain errors like sqrt(-1), it'll do so for atanh/asinh/acosh. And another platform that signals a floating-point exception for sqrt(-1) will do the same for atanh(3).
I've left in the "huge+x > 1.0" test in asinh; it's kinda pointless, but also fairly harmless.
I think its only point is to make sure that the inexact flag gets set where appropriate, and
Python doesn't much care about the inexact flag.
I'm reasonably sure that the test "x == 1." in acosh is safe.
Mark Dickinson wrote:
- it doesn't touch errno, but lets the platform decide how to handle errors (i.e. produce a special value/set errno/signal a floating-point exception/some combination of these). This will make the asinh, acosh, atanh functions behave in the same way that the regular libm functions behave on any platform. So e.g. if a particular platform is used to setting errno for domain errors like sqrt(-1), it'll do so for atanh/asinh/acosh. And another platform that signals a floating-point exception for sqrt(-1) will do the same for atanh(3).
I tried your patch. It fixes the problem with atanh0022 and 0023 test but it breaks in other places. math.acosh(0) returns NaN and does NOT raise an exception. math.atanh(1) raises OverflowError instead of ValueError.
The libm of uclibc *does* set errno for signaling NaN. The relevant code is in w_acos.c and k_standard.c. I don't know how emit a signal for a NaN but setting errno sounds reasonable for me and it gives the desired output.
w_acosh.c:
double acosh(double x) /* wrapper acosh */
{
#ifdef _IEEE_LIBM
return __ieee754_acosh(x);
#else
double z;
z = __ieee754_acosh(x);
if(_LIB_VERSION == _IEEE_ || isnan(x)) return z;
if(x<1.0) {
return __kernel_standard(x,x,29); /* acosh(x<1) */
} else
return z;
#endif
}
kernel_standard:
case 129:
/* acosh(x<1) */
exc.type = DOMAIN;
exc.name = type < 100 ? "acosh" : "acoshf";
exc.retval = zero/zero;
if (_LIB_VERSION == _POSIX_)
errno = EDOM;
else if (!matherr(&exc)) {
if (_LIB_VERSION == _SVID_) {
(void) WRITE2("acosh: DOMAIN error\n", 20);
}
errno = EDOM;
}
break;
Hmmm.
For atanh(1): raising OverflowError is actually consistent with what currently happens.
The only singularity that's already present in math is log(0), and it seems that that
raises OverflowError on OS X, Linux and Windows... I wonder whether this is what Tim
meant to say?
For acosh(0): you're right, and I'm wrong---this should definitely return a NaN and set errno. I guess that dividing 0 by 0 doesn't set errno on Windows. Okay: let's set it directly there.
I do still think that asinh(nan), atanh(nan) and acosh(nan) should return nan and not raise any exceptions, just for the sake of consistency with Linux/OS X and with the other libm functions.
I guess I don't really care about asinh(+/-inf), etc: an infinite return value will be caught by the stuff in math_1 anyway.
Mark Dickinson wrote:
For atanh(1): raising OverflowError is actually consistent with what currently happens.
The only singularity that's already present in math is log(0), and it seems that that raises OverflowError on OS X, Linux and Windows... I wonder whether this is what Tim meant to say?
But on Linux atanh(1) sets errno = EDOM and raises a ValueError. Either the outcome is not consistent or the libm developers have a different philosophy than Time and you. *g*
For acosh(0): you're right, and I'm wrong---this should definitely return a NaN and set errno. I guess that dividing 0 by 0 doesn't set errno on Windows. Okay: let's set it directly there.
I'm not an expert on IEEE754 and NaN but I guess "Signaling NaN" also means it sets errno. Personally I'm not interested in reimplementing or fixing Windows' mathematical library (although it's tempting *g*) but to get the results right for our purpose. You are right, we should try to mimic the C99 standard whenever it is possible. But it's more important to get the functions of our math module right.
I do still think that asinh(nan), atanh(nan) and acosh(nan) should return nan and not raise any exceptions, just for the sake of consistency with Linux/OS X and with the other libm functions.
Yes, it makes perfectly sense and glibc's libm already does the right thing. However Windows screws up again so I've to fix the function call manually. Damn you Windows!
I inserted the four lines in mathmodule.c:math_1() and something similar
in math_2() to fix Windows.
#ifndef __GNUC__ /* Windows et al */
if (Py_IS_NAN(x))
return PyFloat_FromDouble(x+x);
#endif
I guess I don't really care about asinh(+/-inf), etc: an infinite return value will be caught by the stuff in math_1 anyway.
Are you fine with OverflowError?
I'm going to create a branch for your work. It's getting tedious sending large patches back and forth.
Christian
The patch was part of the merge of Mark's and my trunk-math branch. It was merged into the trunk and 3.0 a while ago.
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: ```python assignee = None closed_at =
created_at =
labels = ['extension-modules', 'type-feature']
title = 'Enhancements for mathmodule'
updated_at =
user = 'https://github.com/tiran'
```
bugs.python.org fields:
```python
activity =
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date =
closer = 'christian.heimes'
components = ['Extension Modules']
creation =
creator = 'christian.heimes'
dependencies = ['1635']
files = ['8978', '8985', '9251', '9252']
hgrepos = []
issue_num = 1640
keywords = ['patch']
message_count = 56.0
messages = ['58694', '58698', '58699', '58702', '58706', '58735', '58749', '58758', '58770', '58786', '59115', '59137', '59138', '59152', '59153', '59154', '59155', '59156', '59165', '59166', '59211', '60257', '60259', '60264', '61290', '61306', '61308', '61309', '61310', '61311', '61366', '61367', '61370', '61371', '61372', '61376', '61385', '61386', '61394', '61398', '61399', '61401', '61402', '61403', '61414', '61419', '61423', '61431', '61441', '61442', '61443', '61444', '61474', '61479', '61498', '66047']
nosy_count = 5.0
nosy_names = ['tim.peters', 'mark.dickinson', 'Rhamphoryncus', 'christian.heimes', 'gmcastil']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1640'
versions = ['Python 2.6', 'Python 3.0']
```