Fix nan handling in fp.mag() and hyper() #688
Conversation
mpmath/ctx_fp.py
Outdated
| @@ -200,7 +203,7 @@ def hypsum(ctx, p, q, types, coeffs, z, maxterms=6000, **kwargs): | |||
| for i in num: t *= (coeffs[i]+k) | |||
| for i in den: t /= (coeffs[i]+k) | |||
| k += 1; t /= k; t *= z; s += t | |||
| if abs(t) < tol: | |||
| if not abs(t) >= tol: | |||
There was a problem hiding this comment.
Why is this needed? To account for a possible NaN from abs(t)?
There was a problem hiding this comment.
Please check for NaN explicitly, for code readability.
There was a problem hiding this comment.
Ok, a quick check for nan was added to the hyper(), everything else was reverted.
There was a problem hiding this comment.
@dimpase, does it make sense for you now?
There was a problem hiding this comment.
it's still puzzling how it's possible to get to the NaN check without getting an overflow (and thus an uncaught error). I.e., why is this check needed, at all?
Also, you can get an inf rather than NaN, and inf is perfectly possible to meaningfully compare with non-infs.
There was a problem hiding this comment.
it's still puzzling how it's possible to get to the NaN check without getting an overflow (and thus an uncaught error). I.e., why is this check needed, at all?
The idea was to use double negation (< -> not >=) to make a quick exit for nan's.
I agree, this is much more clear in form like while abs(t) >= tol: ...
Also, you can get an inf rather than NaN, and inf is perfectly possible to meaningfully compare with non-infs.
I think this piece of code can't handle inf's correctly, so nothing was broken.
Anyway, I've reverted this stuff: an explicit check for nan's in hyper() does the job. The second commit is optional now, but I think it's a good idea to have same output for fp.mag() and mp.mag(). Right now we have 0 and nan for nan's, respectively.
skirpichev
changed the title
Closes mpmath#485 Closes mpmath#479 Closes mpmath#507 Closes mpmath#489 Closes mpmath#488 Closes mpmath#478
|
Ok, given the changes are trivial now, I'm going to merge this tomorrow. |
Closes #485
Closes #479
Closes #507
Closes #489
Closes #488
Closes #478