GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101678: refactor the math module to use special functions from c11 by skirpichev · Pull Request #101679 · python/cpython

Conversation

@skirpichev

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Shouldn't affect users, hence no news.

  • Issue: Refactor math module to use C99+ special functions (erf, erfc, etc) #101678

Automerge-Triggered-By: GH:mdickinson

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@skirpichev
pythongh-101678: refactor the math module to use special functions fr…
d202b5c
…om c11
@bedevere-bot bedevere-bot added the awaiting review label Feb 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 8, 2023
Refactor math module to use C99+ special functions (erf, erfc, etc) #101678
Closed
@arhadthedev arhadthedev added the extension-modules C modules in the Modules dir label Feb 8, 2023
@skirpichev
Use libc's lgamma/tgamma instead of custom implementations
7d14105
@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 8, 2023
@bedevere-bot

bedevere-bot commented Feb 8, 2023

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mdickinson commented Feb 8, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Revert "Use libc's lgamma/tgamma instead of custom implementations"
468096b
This reverts commit 7d14105.
@mdickinson
mdickinson self-requested a review February 8, 2023 08:31
@skirpichev

skirpichev commented Feb 8, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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_.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Feb 8, 2023

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

corona10 commented Feb 8, 2023

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits February 8, 2023 12:30
@skirpichev
Comment on custom implementations of tgamma/lgamma
5b75ef8
@skirpichev
UPD commit msg: pythongh-101678: refactor the math module to use erf/…
538810d
…erfc/log2 from c11
@mdickinson

mdickinson commented Feb 8, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson

mdickinson commented Feb 8, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Feb 8, 2023

Copy link
Copy Markdown
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...)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Feb 8, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson mdickinson added the skip news label Feb 8, 2023
mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes
Comment thread Modules/mathmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes
Comment thread Modules/mathmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson

mdickinson commented Feb 8, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed Feb 8, 2023
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@skirpichev @mdickinson
Grammar fix
fa76552
Co-authored-by: Mark Dickinson <[email protected]>
@skirpichev

skirpichev commented Feb 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Restore braces in m_log2 per PEP7
42829cb
@skirpichev
Merge branch 'main' into math-c11
27334d8
@skirpichev

skirpichev commented Feb 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 3 commits February 9, 2023 04:40
@skirpichev
Merge branch 'main' into math-c11
c097545
@skirpichev
Merge math_1_to_whatever() and math_1()
47970e2
@skirpichev
Provide context for log1p workaround
bebbc79
BTW, C11 Annex F says: log1p(±0) returns ±0
@skirpichev
skirpichev requested a review from mdickinson February 9, 2023 02:26
@mdickinson

mdickinson commented Feb 9, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev
arhadthedev reviewed Feb 9, 2023
View reviewed changes
Comment thread Modules/_math.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson

mdickinson commented Feb 9, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits February 9, 2023 11:06
@skirpichev
Revert "Merge math_1_to_whatever() and math_1()"
ee5e689
This reverts commit 47970e2.
@skirpichev
Fix issues prefixes
06e9c8c
@skirpichev

skirpichev commented Feb 9, 2023

Copy link
Copy Markdown
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.

arhadthedev reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson approved these changes Feb 9, 2023
View reviewed changes

@mdickinson mdickinson left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Feb 9, 2023
@mdickinson mdickinson added the 🤖 automerge label Feb 9, 2023
@skirpichev

skirpichev commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

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

Aside from some tiny performance bonus.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev skirpichev mentioned this pull request Feb 9, 2023
gh-101678: Merge math_1_to_whatever() and math_1() #101730
Merged
@miss-islington

miss-islington commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington
miss-islington merged commit 5839575 into python:main Feb 9, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 9, 2023
@skirpichev
skirpichev deleted the math-c11 branch February 9, 2023 08:45
@mdickinson

mdickinson commented Feb 9, 2023

Copy link
Copy Markdown
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. :-(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

vstinner commented Apr 5, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Oct 5, 2026
gh-153144: Avoid checking errno for atan2 #153148
Open
Remove m_remainder() #158826
Closed
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
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@skirpichev @bedevere-bot @mdickinson @corona10 @miss-islington @vstinner @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.