bpo-33089: Multidimensional math.hypot()#8474
Conversation
tim-one
left a comment
tim-one
left a comment
There was a problem hiding this comment.
Looks useful to me! Left some minor comments.
|
|
||
| # 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)?
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)".
Lib/test/test_math.py
Outdated
| # 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.
There was a problem hiding this comment.
Okay, changed to a 3-4-5 triangle.
Modules/mathmodule.c
Outdated
| } | ||
| 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.
There was a problem hiding this comment.
Okay, added the -1.0 test.
Modules/mathmodule.c
Outdated
| 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
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
* 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.
4 participants
https://bugs.python.org/issue33089