gh-101410: Improve error messages in math module by CharlieZhao95 · Pull Request #101492 · python/cpython

CharlieZhao95 · 2023-02-01T09:09:19Z

Make several math functions support custom error messages.
math.sqrt() and math.log() now provide more helpful error messages.

>>> import math
# before
>>> math.sqrt(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math domain error

# now
>>> math.sqrt(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math.sqrt() expects a non-negative input, got '-1'.
See cmath.sqrt() for variation that supports complex numbers

>>> math.log(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math.log() expects a positive input, got '0'.

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

OTheDev · 2023-02-02T05:23:53Z

Modules/mathmodule.c

-      "Return the square root of x.")
+      "Return the square root of x.",
+      "math.sqrt() expects a non-negative input, got '%R'.\n"
+      "See cmath.sqrt() for variation that supports complex numbers")


Perhaps an a before variation would be more correct? Or

See cmath.sqrt(), which also supports negative inputs. I don't know what's better - just a suggestion (:

Perhaps an a before variation would be more correct? Or

See cmath.sqrt(), which also supports negative inputs. I don't know what's better - just a suggestion (:

Thanks! It seems reasonable to me to add a. Let's wait for others to reply. :)

CharlieZhao95 · 2023-02-03T02:18:45Z

I guess similar functions and macros can have the same changes (eg FUNC1A, FUNC2). I will continue my work after the initial review. :)

Modules/mathmodule.c

…h_error_message

skirpichev · 2023-02-18T12:57:40Z

Modules/mathmodule.c

    if (Py_IS_NAN(r) && !Py_IS_NAN(x)) {
-        PyErr_SetString(PyExc_ValueError,
-                        "math domain error"); /* invalid arg */
+        PyErr_Format(PyExc_ValueError, err_msg, arg); /* invalid arg */


Shouldn't same message be used below on the line 1076 as well (infinite r for finite input)? atanh(-1) is an example.

Shouldn't same message be used below on the line 1076 as well (infinite r for finite input)? atanh(-1) is an example.

I was wondering if there is a function that can have both input and output( NAN r for non-NAN x and infinite r for finite x).

If such a function exists, it may lead to confusing error messages, because one error message corresponds to two scenarios. I haven't thought of such a function yet, please suggest if it exists. :)

Same atanh :)

atanh(2) - for the 1st scenario and atanh(1) - for the second. I doubt it will be confusing if you raise instead something like ValueError("atanh expects an argument from (-1, -1), got %R"). I don't think it does make sense to have a different descriptions for pole errors (like atanh(1)). @mdickinson ?

See 9c32ae0 from #102523.

serhiy-storchaka

Would not be better to output the converted C double value x instead of the original Python object arg? The repr of the Python object can be arbitrary long, unlike to the string representation of C double. Also, there may be some errors introduced at converting Python object to C double, not visible from its repr. Although it is much more cumbersome.

serhiy-storchaka · 2023-09-23T16:20:41Z

For example, math.log10(Fraction(1, 10**400)) raises an exception, even if the argument is positive. On other hand math.sqrt(Fraction(-1, 10**400)) returns -0.0 even if the argument is negative. BTW, math.sqrt() returning negative zero can cause issues in formatted output.

CharlieZhao95 · 2023-09-26T08:33:39Z

Would not be better to output the converted C double value x instead of the original Python object arg?

Thanks for your suggestion! I forgot this PR while waiting for others to review it, and it’s time to pick it up :)

IMO, it is better to use the C double variable. The only drawback is that we will lose some floating point precision in error message. Does this confuse others? Perhaps this minor flaw is acceptable.

# Using double(%f), the default value is six decimal places.
>>> math.sqrt(-math.pi)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math.sqrt() expects a nonnegative input, got '-3.141593'.
See cmath.sqrt() for a variation that supports complex numbers
>>> math.pi
3.141592653589793

>>> math.sqrt(-0.00000001)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math.sqrt() expects a nonnegative input, got '-0.000000'.
See cmath.sqrt() for a variation that supports complex numbers

AA-Turner · 2023-09-26T09:52:31Z

ValueError: math.sqrt() expects a nonnegative input, got '-3.141593'.

Perhaps just append ' (rounded)' to the end of the line?

A

skirpichev · 2023-09-26T10:14:09Z

In most cases functions in the module are 1-arg. I think that if we show the function domain (e.g. math.sqrt() expects a non-negative input.) - this will be more than enough.

serhiy-storchaka · 2023-09-26T10:52:33Z

Of course, the output should be more precise and consistent with Python representation, so do not use snprintf() or like. There are several ways to do this:

Create a Python float object with PyFloat_FromDouble, then use %R format.
Use PyOS_double_to_string() (as in the implementation of float.__repr__), then use %s format to insert it in the error message.

Before writing code, wait to see if other core developers (in particular @mdickinson and @tim-one) have something to say about this idea.

skirpichev · 2024-07-21T09:52:30Z

FYI, @CharlieZhao95, this branch has unresolved conflicts. Are you planing to finish this? If not, I'll open a pr, based on this work.

CharlieZhao95 · 2024-07-22T07:25:56Z

FYI, @CharlieZhao95, this branch has unresolved conflicts. Are you planing to finish this? If not, I'll open a pr, based on this work.

Thanks for reminding! BTW, is it time to continue this PR? Maybe we can make this PR simpler (remove got '%R' from the error message first)

picnixz

Can you add tests for checking the new messages?

Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.Dt2aQE.rst

Modules/mathmodule.c

picnixz · 2024-07-22T07:33:03Z

Modules/mathmodule.c

        return NULL;

-    num = loghelper(args[0], m_log);
+    num = loghelper(args[0], m_log, "math.log() expects a positive input, got '%R'.");


I don't think using quotes around numbers is relevant. For instance, if it's a custom class of a float that overrides __repr__, then you don't necessarily want to surround the __repr__ by quotes.

Suggested change

num = loghelper(args[0], m_log, "math.log() expects a positive input, got '%R'.");

num = loghelper(args[0], m_log, "math.log() expects a positive input, got %R.");

picnixz · 2024-07-22T07:33:08Z

Modules/mathmodule.c

 /*[clinic end generated code: output=5425899a4d5d6acb input=08321262bae4f39b]*/
 {
-    return loghelper(x, m_log2);
+    return loghelper(x, m_log2, "math.log2() expects a positive input, got '%R'.");


picnixz · 2024-07-22T07:33:40Z

Modules/mathmodule.c

 /*[clinic end generated code: output=be72a64617df9c6f input=b2469d02c6469e53]*/
 {
-    return loghelper(x, m_log10);
+    return loghelper(x, m_log10, "math.log10() expects a positive input, got '%R'.");


…t2aQE.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

CharlieZhao95 · 2024-07-22T08:05:44Z

Can you add tests for checking the new messages?

Sure :) I'm waiting for other core developers to reply. I will add tests after they finalize the solution.

skirpichev · 2024-10-15T04:37:03Z

This work continued in #124299

CharlieZhao95 added 3 commits February 1, 2023 11:02

Customize error messages in the math module.

85edd15

Imporve error messages for math.log().

2448fe4

Add a NEWS entry.

c0c1e2d

bedevere-bot added the awaiting review label Feb 1, 2023

bedevere-bot mentioned this pull request Feb 1, 2023

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

Closed

add new line

1a491f2

OTheDev reviewed Feb 2, 2023

View reviewed changes

CharlieZhao95 marked this pull request as ready for review February 3, 2023 02:19

mdickinson reviewed Feb 5, 2023

View reviewed changes

Modules/mathmodule.c Outdated Show resolved Hide resolved

CharlieZhao95 added 2 commits February 6, 2023 10:00

fix nit

4cd75b0

Merge remote-tracking branch 'upstream/main' into pythongh-101410/mat…

611dedb

…h_error_message

skirpichev reviewed Feb 18, 2023

View reviewed changes

serhiy-storchaka reviewed Sep 23, 2023

View reviewed changes

Merge branch 'main' into pythongh-101410/math_error_message

ef68c24

Merge branch 'main' into pythongh-101410/math_error_message

dc8e593

picnixz reviewed Jul 22, 2024

View reviewed changes

CharlieZhao95 and others added 3 commits July 22, 2024 15:43

Update Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.D…

75a7c5c

…t2aQE.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Update Modules/mathmodule.c

4b2ec23

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Update Modules/mathmodule.c

3fc73e9

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Merge branch 'main' into pythongh-101410/math_error_message

30be617

skirpichev mentioned this pull request Sep 21, 2024

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

Merged

skirpichev closed this Oct 15, 2024

skirpichev mentioned this pull request May 12, 2025

gh-133904: math.factorial documentation fix #133907

Merged

	num = loghelper(args[0], m_log, "math.log() expects a positive input, got '%R'.");
	num = loghelper(args[0], m_log, "math.log() expects a positive input, got %R.");

Uh oh!

Conversation

CharlieZhao95 commented Feb 1, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 commented Feb 3, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharlieZhao95 commented Sep 26, 2023

Uh oh!

AA-Turner commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Sep 26, 2023

Uh oh!

serhiy-storchaka commented Sep 26, 2023

Uh oh!

skirpichev commented Jul 21, 2024

Uh oh!

CharlieZhao95 commented Jul 22, 2024

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlieZhao95 commented Jul 22, 2024

Uh oh!

skirpichev commented Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

CharlieZhao95 commented Feb 1, 2023 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Sep 23, 2023 •

edited

Loading

AA-Turner commented Sep 26, 2023 •

edited

Loading