Skip to content
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

Jump to bottom

gh-101678: refactor the math module to use special functions from c11 #101679

Merged
merged 13 commits into from
Feb 9, 2023
Merged

gh-101678: refactor the math module to use special functions from c11 #101679

merged 13 commits into from
Feb 9, 2023

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Feb 8, 2023 •
edited by miss-islington
Loading

Shouldn't affect users, hence no news.

Automerge-Triggered-By: GH:mdickinson

@bedevere-bot bedevere-bot mentioned this pull request Feb 8, 2023
Closed
@arhadthedev arhadthedev added the extension-modules C modules in the Modules dir label Feb 8, 2023
@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 8, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 7d14105 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 8, 2023
@mdickinson
Copy link
Member

What's the benefit of doing this? The downside is that we potentially introduce a quality regression on some platforms.

@mdickinson mdickinson self-requested a review February 8, 2023 08:31
@skirpichev
Copy link
Member Author

skirpichev commented Feb 8, 2023 •
edited
Loading

What's the benefit of doing this?

Less C code to maintain, perhaps?

BTW, for erf/erfc there are no failures so far.

we potentially introduce a quality regression on some platforms

It's not a for error functions, because platforms claims that they support C11 and libm's versions will play the game anyway ;) There is no such preprocessor flag as _I_M_A_BROKEN_LIBM_.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 8, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 468096b 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 8, 2023
@corona10
Copy link
Member

corona10 commented Feb 8, 2023

What's the benefit of doing this? The downside is that we potentially introduce a quality regression on some platforms.

If the patch satisfies C11 compiler requirement and PEP 11 support, it could be acceptable.

@mdickinson
Copy link
Member

mdickinson commented Feb 8, 2023 •
edited
Loading

@corona10 Yes, the "this" in my comment could have been clearer.

So long as we keep this PR focused on removing what's now effectively dead code (the code that depends on HAVE_LOG2, HAVE_ERF, etc. not being defined), that's fine. It's the replacement of our own gamma and lgamma code that should be avoided - we'd be replacing tested, working code with code of unknown quality.

FWIW, speaking as the original author of the gamma and lgamma functions and maintainer of the math module, the maintenance burden from that code has been essentially zero since it was originally added.

@mdickinson
Copy link
Member

@skirpichev Thanks for the PR! I'll have time to review properly this evening (UTC+0).

@skirpichev
Copy link
Member Author

So long as we keep this PR focused on removing what's now effectively dead code

That was essentially the target of the issue.

Thank you for review and I'm sorry for a missed issue/pr (adding c11 keyword to search was too restrictive this time...)

@skirpichev
Copy link
Member Author

Maybe using math_1 wrapper for erf/erfc is more safe?

mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes
Modules/mathmodule.c Outdated Show resolved Hide resolved
mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes
Modules/mathmodule.c Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

Maybe using math_1 wrapper for erf/erfc is more safe?

I don't think it's needed, since erf and erfc can't overflow, and are valid over the entire real line.

mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM aside from two tiny nitpicks.

@skirpichev @mdickinson
Grammar fix
fa76552 
Co-authored-by: Mark Dickinson <[email protected]>
@skirpichev
Copy link
Member Author

skirpichev commented Feb 9, 2023 •
edited
Loading

I don't think it's needed, since erf and erfc can't overflow, and are valid over the entire real line.

Same is true to the tanh, for example, isn't?

@skirpichev
Copy link
Member Author

skirpichev commented Feb 9, 2023 •
edited
Loading

I would suggest merging math_1 and math_1_to_whatever (not used anywhere else). The comment before math_1_to_whatever does mention math_1 instead. Ditto for math_1_to_whatever. (One function call less, BTW)

@mdickinson, please review last two commits.

@mdickinson
Copy link
Member

I would suggest merging math_1 and math_1_to_whatever (not used anywhere else).

Sounds reasonable, but for ease of review let's have a separate PR for that, please! I'd like to get this one in, and that gets harder if the scope keeps changing.

arhadthedev
arhadthedev reviewed Feb 9, 2023
View reviewed changes
Modules/_math.h Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

Same is true to the tanh, for example, isn't?

Very likely, yes. Note that that's not a reason to change tanh as it currently is. The libm wrappers portion of the math module is highly stable, and we should be extremely conservative with changes.

@skirpichev
Copy link
Member Author

Sounds reasonable, but for ease of review let's have a separate PR for that

Ok, that part (47970e2) was reverted and I've fixed prefixes for gh issues.

mdickinson
mdickinson approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@mdickinson mdickinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I'll merge when CI completes.

@skirpichev
Copy link
Member Author

Note that that's not a reason to change tanh as it currently is.

Aside from some tiny performance bonus.

@skirpichev skirpichev mentioned this pull request Feb 9, 2023
Merged
@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@miss-islington miss-islington merged commit 5839575 into python:main Feb 9, 2023
@skirpichev skirpichev deleted the math-c11 branch February 9, 2023 08:45
@mdickinson
Copy link
Member

Aargh. And now I remember (again) why I never use the auto-merge label - there's no opportunity to edit the final commit message. :-(

@skirpichev
Copy link
Member Author

Thanks for the hint. Next time I'll adjust pr title/body to follow review.

carljm added a commit to carljm/cpython that referenced this pull request Feb 9, 2023
@carljm
Merge branch 'main' into inlinecomp2
be3becc 
* main: (82 commits)
  pythongh-101670: typo fix in PyImport_ExtendInittab() (python#101723)
  pythonGH-99293: Document that `Py_TPFLAGS_VALID_VERSION_TAG` shouldn't be used. (#pythonGH-101736)
  no-issue: Add Dong-hee Na as the cjkcodecs codeowner (pythongh-101731)
  pythongh-101678: Merge math_1_to_whatever() and math_1() (python#101730)
  pythongh-101678: refactor the math module to use special functions from c11 (pythonGH-101679)
  pythongh-85984: Remove legacy Lib/pty.py code. (python#92365)
  pythongh-98831: Use opcode metadata for stack_effect() (python#101704)
  pythongh-101283: Version was just released, so should be changed in 3.11.3 (pythonGH-101719)
  pythongh-101283: Fix use of unbound variable (pythonGH-101712)
  pythongh-101283: Improved fallback logic for subprocess with shell=True on Windows (pythonGH-101286)
  pythongh-101277: Port more itertools static types to heap types (python#101304)
  pythongh-98831: Modernize CALL and family (python#101508)
  pythonGH-101696: invalidate type version tag in `_PyStaticType_Dealloc` (python#101697)
  pythongh-100221: Fix creating dirs in `make sharedinstall` (pythonGH-100329)
  pythongh-101670: typo fix in PyImport_AppendInittab() (pythonGH-101672)
  pythongh-101196: Make isdir/isfile/exists faster on Windows (pythonGH-101324)
  pythongh-101614: Don't treat python3_d.dll as a Python DLL when checking extension modules for incompatibility (pythonGH-101615)
  pythongh-100933: Improve `check_element` helper in `test_xml_etree` (python#100934)
  pythonGH-101578: Normalize the current exception (pythonGH-101607)
  pythongh-47937: Note that Popen attributes are read-only (python#93070)
  ...
@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

Nice change.

Shouldn't affect users, hence no news.

In the past, I tried to document these "build changes", just in case if someone is affected. Like:

The C99 functions snprintf() and vsnprintf() are now required to build Python. (Contributed by Victor Stinner in bpo-36020.)

https://docs.python.org/dev/whatsnew/3.10.html#build-changes

@skirpichev
Copy link
Member Author

c11 compiler and stdlib (without optional features) were required since 3.11, so there is nothing new in that sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arhadthedev arhadthedev arhadthedev left review comments

@mdickinson mdickinson mdickinson approved these changes

Assignees
No one assigned
Labels
extension-modules C modules in the Modules dir skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@skirpichev @bedevere-bot @mdickinson @corona10 @miss-islington @vstinner @arhadthedev