Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-101678: refactor the math module to use special functions from c11#101679
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedFeb 8, 2023
🤖 New build scheduled with the buildbot fleet by@mdickinson for commit7d14105 🤖 If you want to schedule another build, you need to add the |
mdickinson commentedFeb 8, 2023
What's the benefit of doing this? The downside is that we potentially introduce a quality regression on some platforms. |
skirpichev commentedFeb 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Less C code to maintain, perhaps? BTW, for erf/erfc there are no failures so far.
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 |
bedevere-bot commentedFeb 8, 2023
🤖 New build scheduled with the buildbot fleet by@mdickinson for commit468096b 🤖 If you want to schedule another build, you need to add the |
corona10 commentedFeb 8, 2023
If the patch satisfies C11 compiler requirement andPEP 11 support, it could be acceptable. |
mdickinson commentedFeb 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 FWIW, speaking as the original author of the |
mdickinson commentedFeb 8, 2023
@skirpichev Thanks for the PR! I'll have time to review properly this evening (UTC+0). |
skirpichev commentedFeb 8, 2023
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 commentedFeb 8, 2023
Maybe using math_1 wrapper for erf/erfc is more safe? |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
mdickinson commentedFeb 8, 2023
I don't think it's needed, since |
mdickinson left a comment
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.
Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
skirpichev commentedFeb 9, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Same is true to the tanh, for example, isn't? |
skirpichev commentedFeb 9, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
mdickinson commentedFeb 9, 2023
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. |
Uh oh!
There was an error while loading.Please reload this page.
mdickinson commentedFeb 9, 2023
Very likely, yes. Note that that'snot a reason to change |
skirpichev commentedFeb 9, 2023
Ok, that part (47970e2) was reverted and I've fixed prefixes for gh issues. |
mdickinson left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
skirpichev commentedFeb 9, 2023
Aside from some tiny performance bonus. |
miss-islington commentedFeb 9, 2023
Status check is done, and it's a success ✅. |
mdickinson commentedFeb 9, 2023
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 commentedFeb 9, 2023
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) ...
vstinner commentedApr 5, 2023
Nice change.
In the past, I tried to document these "build changes", just in case if someone is affected. Like:
https://docs.python.org/dev/whatsnew/3.10.html#build-changes |
skirpichev commentedApr 6, 2023
c11 compiler and stdlib (without optional features) were requiredsince 3.11, so there is nothing new in that sense. |
Uh oh!
There was an error while loading.Please reload this page.
Shouldn't affect users, hence no news.
Automerge-Triggered-By: GH:mdickinson