bpo-33089: Multidimensional math.hypot()#8474
Uh oh!
There was an error while loading. Please reload this page.
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