gh-101410: customize error messages for 1-arg math functions#129497
gh-101410: customize error messages for 1-arg math functions#129497vstinner merged 14 commits intopython:mainfrom
Conversation
This also reverts loghelper() change in 75f59bb for integer input. The error message shouldn't include argument value here.
|
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. |
|
@picnixz: Are you available to review this change? Otherwise, I will just merge it. |
picnixz
left a comment
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.
| "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)
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 |
Modules/mathmodule.c
Outdated
| @@ -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.
Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.Dt2aQE.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.Dt2aQE.rst
Outdated
Show resolved
Hide resolved
|
@serhiy-storchaka: Would you mind to review this PR again? |
|
Added whatsnew entry. BTW, feature freeze is soon. If this seems not ready for next release - lets revert also #124299. |
|
We need a @rhettinger's approval, as he is the one who requested the change. |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
@serhiy-storchaka: Do you agree with this change? If yes, would you mind to approve the PR? |
|
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. |
|
see also #132625 |
|
These message look nice. I think users will find them to be helpful when something goes wrong. |
|
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`
5 participants
This also reverts loghelper() change in 75f59bb for integer input. The error message shouldn't include argument value here.