Skip to content

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.

Sign up for GitHub

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

Closed
CharlieZhao95 wants to merge 12 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
85edd15
Customize error messages in the math module.
CharlieZhao95 Feb 1, 2023
2448fe4
Imporve error messages for `math.log()`.
CharlieZhao95 Feb 1, 2023
c0c1e2d
Add a NEWS entry.
CharlieZhao95 Feb 1, 2023
1a491f2
add new line
CharlieZhao95 Feb 1, 2023
4cd75b0
fix nit
CharlieZhao95 Feb 6, 2023
611dedb
Merge remote-tracking branch 'upstream/main' into gh-101410/math_erro…
CharlieZhao95 Feb 6, 2023
ef68c24
Merge branch 'main' into gh-101410/math_error_message
serhiy-storchaka Dec 5, 2023
dc8e593
Merge branch 'main' into gh-101410/math_error_message
CharlieZhao95 Jul 22, 2024
75a7c5c
Update Misc/NEWS.d/next/Library/2023-02-01-16-41-31.gh-issue-101410.D…
CharlieZhao95 Jul 22, 2024
4b2ec23
Update Modules/mathmodule.c
CharlieZhao95 Jul 22, 2024
3fc73e9
Update Modules/mathmodule.c
CharlieZhao95 Jul 22, 2024
30be617
Merge branch 'main' into gh-101410/math_error_message
skirpichev Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Merge branch 'main' into gh-101410/math_error_message
  • Loading branch information
@CharlieZhao95
CharlieZhao95 committed Jul 22, 2024
commit dc8e593e6d56ac5e3fe4e55f5dcc789c0f876875
2 changes: 1 addition & 1 deletion Modules/mathmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,7 +966,7 @@ math_1(PyObject *arg, double (*func) (double), int can_overflow, const char *err
return NULL;
errno = 0;
r = (*func)(x);
if (Py_IS_NAN(r) && !Py_IS_NAN(x)) {
if (isnan(r) && !isnan(x)) {
PyErr_Format(PyExc_ValueError, err_msg, arg); /* invalid arg */

@skirpichev skirpichev Feb 18, 2023

Copy link
Copy Markdown
Member

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.

@CharlieZhao95 CharlieZhao95 Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

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

@skirpichev skirpichev Feb 20, 2023

Copy link
Copy Markdown
Member

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 ?

@skirpichev skirpichev Sep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See 9c32ae0 from #102523.

return NULL;
}
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.