Uh oh!
There was an error while loading. Please reload this page.
gh-101410: customize error messages for 1-arg math functions - #129497
Conversation
This also reverts loghelper() change in 75f59bb for integer input. The error message shouldn't include argument value here.
skirpichev
commented
Jan 31, 2025
skirpichev
commented
Feb 1, 2025
Hmm, in fact I think math.pow() rather should be kept intact. It's domain doesn't look too simple to be described in the error message. |
vstinner
commented
Feb 3, 2025
@picnixz: Are you available to review this change? Otherwise, I will just merge it. |
picnixz
left a comment
There was a problem hiding this comment.
Hard to check when I'm doing the review but we don't have any issue with the '%s' part anymore right?
IIRC we have the following conversions:
- Input is
PyObject*. - Conversion to C double.
- Checks (assuming conversion succeeded).
- On error, use that converted value in the error message (we do it using
PyOS_double_to_str). No possibility of making this very slow since we already have a double that was successfully obtained.
If this is indeed the case, then, modulo the grammar part I am not entirely sure, I'm fine with this change. I like the fact that we report infinite inputs when we expect a finite input by the way.
Uh oh!
There was an error while loading. Please reload this page.
| "The result is between 0 and pi.") | ||
| FUNC1(acosh, acosh, 0, | ||
| "The result is between 0 and pi.", | ||
| "expected a number in range from -1 up to 1, got %s") |
There was a problem hiding this comment.
We could say "a number in the interval [-1, 1]" though I'm a bit worried about the notation that may not be understood (for non mathematicians). Otherwise shouldn't we say "in the range from -1 up to 1" ? (I'm not sure here for the English part so @AA-Turner could help us)
skirpichev
commented
Feb 4, 2025
This issue was related to integer input in the loghelper, that was reverted. In the rest we will show only converted values, i.e.: >>> import math
>>> math.atanh(-2)
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
math.atanh(-2)
~~~~~~~~~~^^^^
ValueError: expected a number between -1 and 1, got -2.0 |
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Feb 5, 2025
| @@ -1229,32 +1233,36 @@ FUNC1AD(gamma, m_tgamma, | |||
| "gamma($module, x, /)\n--\n\n" | |||
| "Gamma function at x.", | |||
| "expected a float or nonnegative integer, got %s") | |||
There was a problem hiding this comment.
I get error for 0, which is a non-negative integer:
>>> math.gamma(0)
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
math.gamma(0)
~~~~~~~~~~^^^
ValueError: expected a float or nonnegative integer, got 0.0I get also error for negative float value which happens to be an integer. The error message is not clear about this.
>>> math.gamma(-1.0)
Traceback (most recent call last):
File "<python-input-2>", line 1, in <module>
math.gamma(-1.0)
~~~~~~~~~~^^^^^^
ValueError: expected a float or nonnegative integer, got -1.0There was a problem hiding this comment.
math.gamma(0)
Thanks, indeed - this should be positive, as for lgamma().
I get also error for negative float value which happens to be an integer. The error message is not clear about this.
I don't see a problem here: the message says we expect nonnegative (>=0) input here (actually, it should be just positive) and you pass a negative integer float value.
There was a problem hiding this comment.
It says "or". -1.0 is a float. "Nonnegative" only applies to "integer".
There was a problem hiding this comment.
-1.0 is an integer in sense of is_integer() predicate or ==.
There was a problem hiding this comment.
But is is also a float. "a or b" is true if "a" is true.
There was a problem hiding this comment.
"expected a noninteger or positive integer, got %s"?
There was a problem hiding this comment.
Something like this, or via the complement of the function domain. It raises ValueError only for non-positive integers, isn't?
OverflowError with generic error message is raised for other values.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Feb 25, 2025
@serhiy-storchaka: Would you mind to review this PR again? |
skirpichev
commented
Mar 29, 2025
Added whatsnew entry. BTW, feature freeze is soon. If this seems not ready for next release - lets revert also #124299. |
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka
commented
Mar 29, 2025
We need a @rhettinger's approval, as he is the one who requested the change. |
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
commented
Apr 14, 2025
@serhiy-storchaka: Do you agree with this change? If yes, would you mind to approve the PR? |
serhiy-storchaka
commented
Apr 14, 2025
I am fine with the current error messages. It was Raymond who requested the addition of details, so it's up to him to decide if this is the right level of details. There's no point in making these changes if they don't satisfy Raymond. |
skirpichev
commented
Apr 17, 2025
see also #132625 |
rhettinger
commented
Apr 18, 2025
These message look nice. I think users will find them to be helpful when something goes wrong. |
skirpichev
commented
Apr 21, 2025
vstinner
commented
Apr 22, 2025
Merged, thank you. |
Followup to #18534 Some more error messages for math functions were changed for Python 3.14, see python/cpython#129497. Fixes `mypyc/test/test_run.py::TestRun::run-math.test::testMathOps`
This also reverts loghelper() change in 75f59bb for integer input. The error message shouldn't include argument value here.