Repository navigation
bpo-33089: Multidimensional math.hypot() - #8474
rhettinger merged 21 commits into
Conversation
Sorry, something went wrong.
tim-one
left a comment
There was a problem hiding this comment.
Looks useful to me! Left some minor comments.
Sorry, something went wrong.
|
|
||
| # Test corner cases | ||
| self.assertEqual(hypot(0.0, 0.0), 0.0) # Max input is zero | ||
| self.assertEqual(hypot(-10.5), 10.5) # Negative input |
There was a problem hiding this comment.
Add self.assertEqual(hypot(), 0.0)?
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, added an explict test for this case. FWIW, this was already tested in the section "Test different numbers of arguments (from zero to five)".
Sorry, something went wrong.
| # Verify scaling for extremely small values | ||
| for exp in range(32): | ||
| scale = FLOAT_MIN / 2.0 ** exp | ||
| self.assertEqual(math.hypot(12*scale, 5*scale), 13*scale) |
There was a problem hiding this comment.
This makes me nervous, because it depends on that
12 * math.sqrt(1 + (5/12)**2)
happens to be exactly 13.0 despite that 5/12 isn't exactly representable. Would be happier with:
self.assertEqual(math.hypot(4*scale, 3*scale), 5*scale)
because every intermediate value in
4 * math.sqrt(1 + (3/4)**2)
is exactly representable in binary.
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, changed to a 3-4-5 triangle.
Sorry, something went wrong.
| } | ||
| for (i=0 ; i<n ; i++) { | ||
| item = PyTuple_GET_ITEM(args, i); | ||
| x = PyFloat_AsDouble(item) / max; |
There was a problem hiding this comment.
Are we quite sure a malicious __float__() implementation can't mutate other values in the argument tuple? That is, that no conversion failed the first time through the tuple guarantees none will fail the second time through?
I don't know. In which case checking for a -1.0 error return here too would be safest.
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, added the -1.0 test.
Sorry, something went wrong.
| x = PyFloat_AsDouble(item) / max; | ||
| csum += x * x; | ||
| } | ||
| return PyFloat_FromDouble(max * sqrt(csum)); // XXX Handle overflow |
There was a problem hiding this comment.
It's good enough for me that +Inf will result from overflow here, but that it is a change from, e.g., what 3.7 does on Windows:
>>> import sys
>>> import math
>>> x = sys.float_info.max
>>> math.hypot(x, x)
Traceback (most recent call last):
...
OverflowError: math range error
Sorry, something went wrong.
There was a problem hiding this comment.
That is good enough for me too :-) I like that it matches the behavior of the pure python code:
>>> import sys >>> coordinates = [sys.float_info.max] * 1 >>> scale * sqrt(sum((x/scale) ** x for x in coordinates)) 1.7976931348623157e+308 >>> coordinates = [sys.float_info.max] * 2 >>> scale * sqrt(sum((x/scale) ** x for x in coordinates)) inf
Sorry, something went wrong.
* Made test for the zero argument case explicit. * Use exactly representable numbers in the extreme small value tests. * Guard against a malicious __float__ that succeeds on the first call and fails on the second.
https://bugs.python.org/issue33089