gh-101678: refactor the math module to use special functions from c11 by skirpichev · Pull Request #101679 · python/cpython · GitHub

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

…om c11

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

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

This reverts commit 7d14105.

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

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

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.

…erfc/log2 from c11

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

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

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

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

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.

LGTM aside from two tiny nitpicks.

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

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?

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.

BTW, C11 Annex F says: log1p(±0) returns ±0

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.

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.

This reverts commit 47970e2.

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.

LGTM. I'll merge when CI completes.

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

Aside from some tiny performance bonus.

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

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

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

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

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

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

pythongh-101678 : refactor the math module to use special functions fr…

d202b5c

…om c11

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

bedevere-bot mentioned this pull request Feb 8, 2023

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

Closed

arhadthedev added the extension-modules C modules in the Modules dir label Feb 8, 2023

Use libc's lgamma/tgamma instead of custom implementations

7d14105

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

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

Revert "Use libc's lgamma/tgamma instead of custom implementations"

468096b

This reverts commit 7d14105.

mdickinson self-requested a review February 8, 2023 08:31

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

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

skirpichev added 2 commits February 8, 2023 12:30

Comment on custom implementations of tgamma/lgamma

5b75ef8

UPD commit msg: pythongh-101678 : refactor the math module to use erf/…

538810d

…erfc/log2 from c11

mdickinson added the skip news label Feb 8, 2023

mdickinson reviewed Feb 8, 2023

View reviewed changes

Comment thread Modules/mathmodule.c Outdated

mdickinson reviewed Feb 8, 2023

View reviewed changes

Comment thread Modules/mathmodule.c Outdated

mdickinson reviewed Feb 8, 2023

View reviewed changes

Grammar fix

fa76552

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

Restore braces in m_log2 per PEP7

42829cb

Merge branch 'main' into math-c11

27334d8

skirpichev added 3 commits February 9, 2023 04:40

Merge branch 'main' into math-c11

c097545

Merge math_1_to_whatever() and math_1()

47970e2

Provide context for log1p workaround

bebbc79

BTW, C11 Annex F says: log1p(±0) returns ±0

skirpichev requested a review from mdickinson February 9, 2023 02:26

arhadthedev reviewed Feb 9, 2023

View reviewed changes

Comment thread Modules/_math.h Outdated

skirpichev added 2 commits February 9, 2023 11:06

Revert "Merge math_1_to_whatever() and math_1()"

ee5e689

This reverts commit 47970e2.

Fix issues prefixes

06e9c8c

mdickinson approved these changes Feb 9, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 9, 2023

mdickinson added the 🤖 automerge label Feb 9, 2023

skirpichev mentioned this pull request Feb 9, 2023

gh-101678: Merge math_1_to_whatever() and math_1() #101730

Merged

miss-islington merged commit 5839575 into python:main Feb 9, 2023

bedevere-bot removed the awaiting merge label Feb 9, 2023

skirpichev deleted the math-c11 branch February 9, 2023 08:45

gh-101678: refactor the math module to use special functions from c11 by skirpichev · Pull Request #101679 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-101678: refactor the math module to use special functions from c11#101679miss-islington merged 13 commits intopython:mainpython/cpython:mainfrom skirpichev:math-c11skirpichev/cpython:math-c11Copy head branch name to clipboard

Conversation

skirpichev commented Feb 8, 2023 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Feb 8, 2023

Uh oh!

mdickinson commented Feb 8, 2023

Uh oh!

skirpichev commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Feb 8, 2023

Uh oh!

corona10 commented Feb 8, 2023

Uh oh!

mdickinson commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdickinson commented Feb 8, 2023

Uh oh!

skirpichev commented Feb 8, 2023

Uh oh!

skirpichev commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

mdickinson commented Feb 8, 2023

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdickinson commented Feb 9, 2023

Uh oh!

Uh oh!

mdickinson commented Feb 9, 2023

Uh oh!

skirpichev commented Feb 9, 2023

Uh oh!

mdickinson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev commented Feb 9, 2023

Uh oh!

miss-islington commented Feb 9, 2023

Uh oh!

mdickinson commented Feb 9, 2023

Uh oh!

skirpichev commented Feb 9, 2023

Uh oh!

vstinner commented Apr 5, 2023

Uh oh!

skirpichev commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gh-101678: refactor the math module to use special functions from c11#101679
miss-islington merged 13 commits into
python:mainfrom
skirpichev:math-c11

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

skirpichev commented Feb 8, 2023 •
edited
Loading

mdickinson commented Feb 8, 2023 •
edited
Loading

skirpichev commented Feb 9, 2023 •
edited
Loading

skirpichev commented Feb 9, 2023 •
edited
Loading

mdickinson left a comment •
edited
Loading