Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-101410: customize error messages for 1-arg math functions#129497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
skirpichev commented Jan 31, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
This also reverts loghelper() change in 75f59bb for integer input. The error message shouldn't include argument value here.
skirpichev commented Jan 31, 2025
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Modules/mathmodule.c Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It says "or". -1.0 is a float. "Nonnegative" only applies to "integer".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1.0 is an integer in sense of is_integer() predicate or ==.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is is also a float. "a or b" is true if "a" is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"expected a noninteger or positive integer, got %s"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.Dt2aQE.rst Outdated Show resolvedHide resolved
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. |
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
d0660a9 into python:mainUh oh!
There was an error while loading. Please reload this page.
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.