NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.7k

gh-101410: Improve error messages in math module#101492

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

CharlieZhao95 wants to merge12 commits intopython:mainfromCharlieZhao95:gh-101410/math_error_message

Closed

gh-101410: Improve error messages in math module#101492

CharlieZhao95 wants to merge12 commits intopython:mainfromCharlieZhao95:gh-101410/math_error_message

Conversation

Copy link

Contributor

CharlieZhao95 commentedFeb 1, 2023•
edited by bedevere-bot
Loading

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

>>>importmath# before>>>math.sqrt(-1)Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:mathdomainerror# now>>>math.sqrt(-1)Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:math.sqrt()expectsanon-negativeinput,got'-1'.Seecmath.sqrt()forvariationthatsupportscomplexnumbers>>>math.log(0)Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:math.log()expectsapositiveinput,got'0'.

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

CharlieZhao95 added3 commits

February 1, 2023 11:02

Customize error messages in the math module.

85edd15

Imporve error messages formath.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

Modules/mathmodule.c Outdated

		"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")

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Perhaps ana beforevariation would be more correct? Or

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Perhaps ana beforevariation 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 adda. Let's wait for others to reply. :)

Copy link

ContributorAuthor

CharlieZhao95 commentedFeb 3, 2023

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

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 OutdatedShow resolvedHide resolved

CharlieZhao95 added2 commits

February 6, 2023 10:00

fix nit

4cd75b0

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

611dedb

…h_error_message

skirpichev reviewed

Feb 18, 2023

View reviewed changes

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 */

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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. :)

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 ?

Copy link

Contributor

skirpichevSep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

See9c32ae0 from#102523.

serhiy-storchaka reviewed

Sep 23, 2023

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would not be better to output the converted C double valuex instead of the original Python objectarg? 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.

Copy link

Member

serhiy-storchaka commentedSep 23, 2023•
edited
Loading

For example,math.log10(Fraction(1, 10**400)) raises an exception, even if the argument is positive. On other handmath.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.

Copy link

ContributorAuthor

CharlieZhao95 commentedSep 26, 2023

Would not be better to output the converted C double valuex instead of the original Python objectarg?

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 (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:math.sqrt()expectsanonnegativeinput,got'-3.141593'.Seecmath.sqrt()foravariationthatsupportscomplexnumbers>>>math.pi3.141592653589793>>>math.sqrt(-0.00000001)Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>ValueError:math.sqrt()expectsanonnegativeinput,got'-0.000000'.Seecmath.sqrt()foravariationthatsupportscomplexnumbers

Copy link

Member

AA-Turner commentedSep 26, 2023•
edited
Loading

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

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

Copy link

Contributor

skirpichev commentedSep 26, 2023

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.

Copy link

Member

serhiy-storchaka commentedSep 26, 2023

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

Create a Python float object withPyFloat_FromDouble, then use %R format.
UsePyOS_double_to_string() (as in the implementation offloat.__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.

Merge branch 'main' intopythongh-101410/math_error_message

ef68c24

Copy link

Contributor

skirpichev commentedJul 21, 2024

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

Merge branch 'main' intopythongh-101410/math_error_message

dc8e593

Copy link

ContributorAuthor

CharlieZhao95 commentedJul 22, 2024

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 (removegot '%R' from the error message first)

picnixz reviewed

Jul 22, 2024

View reviewed changes

Copy link

Member

picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 OutdatedShow resolvedHide resolved

Modules/mathmodule.c OutdatedShow resolvedHide resolved

Modules/mathmodule.c

		returnNULL;

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

Copy link

Member

picnixzJul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.");

Modules/mathmodule.c

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

Copy link

Member

picnixzJul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ditto.

Modules/mathmodule.c

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

Copy link

Member

picnixzJul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ditto

CharlieZhao95and others added3 commits

July 22, 2024 15:43

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

75a7c5c

…t2aQE.rstCo-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>

Copy link

ContributorAuthor

CharlieZhao95 commentedJul 22, 2024

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.

Merge branch 'main' intopythongh-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

Copy link

Contributor

skirpichev commentedOct 15, 2024

This work continued in#124299

skirpichev closed this

Oct 15, 2024

skirpichev mentioned this pull request

May 12, 2025

gh-133904: math.factorial documentation fix#133907

Merged

Labels

awaiting review

Movatterモバイル変換

Uh oh!

gh-101410: Improve error messages in math module#101492

gh-101410: Improve error messages in math module#101492

Uh oh!

Conversation

CharlieZhao95 commentedFeb 1, 2023• edited by bedevere-botLoading 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 commentedFeb 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 commentedSep 23, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

CharlieZhao95 commentedSep 26, 2023

Uh oh!

AA-Turner commentedSep 26, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

skirpichev commentedSep 26, 2023

Uh oh!

serhiy-storchaka commentedSep 26, 2023

Uh oh!

skirpichev commentedJul 21, 2024

Uh oh!

CharlieZhao95 commentedJul 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 commentedJul 22, 2024

Uh oh!

skirpichev commentedOct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

CharlieZhao95 commentedFeb 1, 2023•
edited by bedevere-bot
Loading

serhiy-storchaka commentedSep 23, 2023•
edited
Loading

AA-Turner commentedSep 26, 2023•
edited
Loading