gh-101410: support custom messages for domain errors in the math module #124299

skirpichev · 2024-09-21T09:22:09Z

This adds basic support to override default messages for domain errors in the math_1() helper. The sqrt(), atanh(), log2(), log10() and log() functions were modified as examples. New macro supports gradual changing of error messages in other 1-arg functions.

Issue: Customize and improve error messages in the math module. #101410

…h module This adds basic support to override default messages for domain errors in the math_1() helper. The sqrt(), atanh(), log2(), log10() and log() functions were modified as examples. New macro supports gradual changing of error messages in other 1-arg functions. Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

skirpichev · 2024-09-21T10:00:40Z

#101492 somehow stuck, here I would like to continue this work. @CharlieZhao95, are you ok with this?

The given pr introduces a new macro and change only few functions, that covers all cases in math_1(). I hope, this also address reviews in the mentioned pr (tests added, error messages print converted argument, etc).

I kept only short error messages. Not sure if it worth printing the function name, as it usually shown in tracebacks.

>>> import math
>>> for i in range(5, -10, -2):
...     print(math.log(i))
...     
1.6094379124341003
1.0986122886681098
0.0
Traceback (most recent call last):
  File "<python-input-1>", line 2, in <module>
    print(math.log(i))
          ~~~~~~~~^^^
ValueError: expected a positive input, got -1

Not sure also if we must suggest cmath in error messages. But if we decide to do so, probably it does make sense for almost every domain error in the math module and we can do this with a template.

diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c
index 665b02e702..c4516fabda 100644
--- a/Modules/mathmodule.c
+++ b/Modules/mathmodule.c
@@ -1046,7 +1046,11 @@ math_2(PyObject *const *args, Py_ssize_t nargs,
 
 #define FUNC1D(funcname, func, can_overflow, docstring, err_msg)        \
     static PyObject * math_##funcname(PyObject *self, PyObject *args) { \
-        return math_1(args, func, can_overflow, err_msg);               \
+        return math_1(args, func, can_overflow,                         \
+                      (err_msg                                          \
+                       "\nSee cmath."                                   \
+                       #funcname                                        \
+                       "(), that supports complex numbers"));           \
     }\
     PyDoc_STRVAR(math_##funcname##_doc, docstring);

CC @rhettinger as author of the issue, I would appreciate feedback
CC @serhiy-storchaka

picnixz

Looks good to me. Just some nitpicks.

picnixz · 2024-09-22T07:57:27Z

Modules/mathmodule.c

+ return math_1(args, func, can_overflow, NULL); \
+ }\
+ PyDoc_STRVAR(math_##funcname##_doc, docstring);


Could you align line continuations here please?

picnixz · 2024-09-22T08:01:44Z

Modules/mathmodule.c

@@ -2229,11 +2244,11 @@ math_log(PyObject *module, PyObject * const *args, Py_ssize_t nargs)
 if (!_PyArg_CheckPositional("log", nargs, 1, 2))
 return NULL;

- num = loghelper(args[0], m_log);
+ num = loghelper(args[0], m_log, "expected a positive input, got %R");


Could we use "strictly positive" instead of "positive" input here?

What we will gain with a more longer variant? I don't see difference.

skirpichev · 2024-09-22T08:07:26Z

Modules/mathmodule.c

+ return math_1(args, func, can_overflow, NULL); \
+ }\


Suggested change

return math_1(args, func, can_overflow, NULL); \

}\

return math_1(args, func, can_overflow, NULL); \

} \

@picnixz, you meant this and ditto for the new macro?

I doubt it's a good idea, see other macros nearby.

serhiy-storchaka

More informative error messages is good, but I afraid that the cost may be too high. If this is really an error due to invalid input data, then some cost is more tolerable. But if the exception is used in the EAFP style of solving computational error (for example, the argument of sqrt() in sqrt(b*b-4*a*c) can be a small negative number due to limited precision computation and can be replaced by 0), additional cost harms the average performance.

Modules/mathmodule.c

serhiy-storchaka · 2024-09-22T11:42:45Z

Modules/mathmodule.c

@@ -2189,8 +2205,7 @@ loghelper(PyObject* arg, double (*func)(double))

 /* Negative or zero inputs give a ValueError. */
 if (!_PyLong_IsPositive((PyLongObject *)arg)) {
- PyErr_SetString(PyExc_ValueError,
- "math domain error");
+ PyErr_Format(PyExc_ValueError, err_msg, arg);


Danger! The argument can be arbitrary negative integer. Creating it decimal representation has superlinear complexity. Even if it could have linear complexity, it can be too long, and the result can be too long. Oh, and normally you will get a ValueError due to the int_max_str_digits limit. In any case the cost of formatting the error message can be too high.

cc @tim-one

Yes, I'll refactor loghelper() case. Probably we can omit argument value in errors from log(). The only problem is that it (unfortunately) has 2-arg form too.

skirpichev · 2024-09-22T12:27:20Z

additional cost harms the average performance.

But it's only printing. Currently, math_1() uses PyFloat_FromDouble() for converted by PyFloat_AsDouble() value; well, maybe I should just use PyOS_double_to_string(). Anyway it doesn't seems to be too expensive in either way.

On another hand, with this - we don't print the original value of the argument (something like Fraction(1/2**1000). Maybe it's better to omit the value at all?

serhiy-storchaka · 2024-09-22T12:37:54Z

If include the value, it should be a converted value, not the original value, because conversion can change the value (for example turn a small positive value into 0). The repr of the original value has also larger chance to produce long and expensive result.

skirpichev · 2024-09-22T12:42:48Z

If include the value, it should be a converted value, not the original value

This is how it works. loghelper() is an exception.

I see only one solution for loghelper(): use two messages. One for PyLong-branch (this message will come from loghelper argument to distinguish arguments of 2-arg form) and a common template for the math_1() fallback.

* use PyOS_double_to_string * drop loghelper arg * don't print argument value in PyLong-branch of loghelper()

serhiy-storchaka

Thank you. You addressed all my concerns. Extreme slow cases were removed, and formatting error messages in common case was optimized.

I would approve this PR, but I want first to know opinion of the author of the request, @rhettinger.

And I think that we can add custom error messages for more functions: acos(), asin(), gamma(), lgamma(). Maybe pow(), where rules are complex.

Lib/test/test_math.py

skirpichev · 2024-09-22T15:19:49Z

And I think that we can add custom error messages for more functions: acos(), asin(), gamma(), lgamma(). Maybe pow(), where rules are complex.

I would prefer to make first required changes in helpers. This pr changes math_1(). Following pr can add custom error messages for all math_1-based functions, it will be mostly mechanical process.

Then we can address e.g. atan2 and pow.

bedevere-app bot added the awaiting review label Sep 21, 2024

bedevere-app bot mentioned this pull request Sep 21, 2024

Customize and improve error messages in the math module. #101410

Open

+1

05b55eb

picnixz reviewed Sep 22, 2024

View reviewed changes

skirpichev commented Sep 22, 2024

View reviewed changes

serhiy-storchaka reviewed Sep 22, 2024

View reviewed changes

address review:

a917cef

* use PyOS_double_to_string * drop loghelper arg * don't print argument value in PyLong-branch of loghelper()

skirpichev requested a review from serhiy-storchaka September 22, 2024 13:23

serhiy-storchaka reviewed Sep 22, 2024

View reviewed changes

Lib/test/test_math.py Outdated Show resolved Hide resolved

Lib/test/test_math.py Show resolved Hide resolved

Lib/test/test_math.py Outdated Show resolved Hide resolved

Lib/test/test_math.py Show resolved Hide resolved

Lib/test/test_math.py Outdated Show resolved Hide resolved

address review: tests (few more, reformatting, etc)

e8f49b0

skirpichev requested a review from picnixz September 22, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-101410: support custom messages for domain errors in the math module #124299

gh-101410: support custom messages for domain errors in the math module #124299

skirpichev commented Sep 21, 2024 •

edited by bedevere-app bot

Loading

skirpichev commented Sep 21, 2024

picnixz left a comment

picnixz Sep 22, 2024

picnixz Sep 22, 2024

skirpichev Sep 22, 2024

skirpichev Sep 22, 2024

skirpichev Sep 22, 2024

serhiy-storchaka left a comment

serhiy-storchaka Sep 22, 2024

skirpichev Sep 22, 2024

skirpichev commented Sep 22, 2024

serhiy-storchaka commented Sep 22, 2024

skirpichev commented Sep 22, 2024

serhiy-storchaka left a comment

skirpichev commented Sep 22, 2024

gh-101410: support custom messages for domain errors in the math module #124299

Are you sure you want to change the base?

gh-101410: support custom messages for domain errors in the math module #124299

Conversation

skirpichev commented Sep 21, 2024 • edited by bedevere-app bot Loading

skirpichev commented Sep 21, 2024

picnixz left a comment

Choose a reason for hiding this comment

picnixz Sep 22, 2024

Choose a reason for hiding this comment

picnixz Sep 22, 2024

Choose a reason for hiding this comment

skirpichev Sep 22, 2024

Choose a reason for hiding this comment

skirpichev Sep 22, 2024

Choose a reason for hiding this comment

skirpichev Sep 22, 2024

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka Sep 22, 2024

Choose a reason for hiding this comment

skirpichev Sep 22, 2024

Choose a reason for hiding this comment

skirpichev commented Sep 22, 2024

serhiy-storchaka commented Sep 22, 2024

skirpichev commented Sep 22, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

skirpichev commented Sep 22, 2024

skirpichev commented Sep 21, 2024 •

edited by bedevere-app bot

Loading