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-131032: Fix math.fma(x, y, z) zero sign by vstinner · Pull Request #131134 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Mar 12, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Fix result sign when z is zero.

  • Issue: test_math.test_fma_zero_result() fails with the musl C library #131032

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner @skirpichev
pythongh-131032: Fix math.fma(x, y, z) zero sign
20d41f1
Fix result sign when z is zero.

Co-Authored-by: Sergey B Kirpichev <[email protected]>
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 12, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Mar 12, 2025
test_math.test_fma_zero_result() fails with the musl C library #131032
Closed
@vstinner
Add NEWS entry
c95e6f9
@skirpichev
skirpichev self-requested a review March 12, 2025 07:46
@skirpichev

skirpichev commented Mar 12, 2025

Copy link
Copy Markdown
Member

!buildbot wasi

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @skirpichev for commit c95e6f9 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

The command will test the builders whose names match following regular expression: wasi

The builders matched are:

  • wasm32-wasi Non-Debug PR
  • wasm32-wasi PR
  • wasm32 WASI 8Core PR
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 12, 2025
@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit c95e6f9 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

If you want to schedule another build, you need to add the 🔨 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 Mar 12, 2025
@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

buildbot/wasm32 WASI 8Core PR — Build done.
buildbot/wasm32-wasi Non-Debug PR — Build done.
buildbot/wasm32-wasi PR — Build done.

Nice, test_math pass on WASI buildbots.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

cc @serhiy-storchaka

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Mar 12, 2025
gh-131032: Add support.linked_to_musl() function #131071
Merged
skirpichev
skirpichev reviewed Mar 12, 2025
View reviewed changes
Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

test_fma_zero_result() failed on Android and FreeBSD.

buildbot/AMD64 Android PR

FAIL: test_fma_zero_result (test.test_math.FMATests.test_fma_zero_result)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_math.py", line 2814, in test_fma_zero_result
    self.assertIsNegativeZero(math.fma(x-y, x+y, -z))
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_math.py", line 2919, in assertIsNegativeZero
    self.assertTrue(
    ~~~~~~~~~~~~~~~^
        value == 0 and math.copysign(1, value) < 0,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        msg="Expected a negative zero, got {!r}".format(value)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
AssertionError: False is not true : Expected a negative zero, got 0.0

buildbot/AMD64 FreeBSD PR

FAIL: test_fma_zero_result (test.test_math.FMATests.test_fma_zero_result)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/pull_request.ware-freebsd/build/Lib/test/test_math.py", line 2814, in test_fma_zero_result
    self.assertIsNegativeZero(math.fma(x-y, x+y, -z))
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
  File "/buildbot/buildarea/pull_request.ware-freebsd/build/Lib/test/test_math.py", line 2919, in assertIsNegativeZero
    self.assertTrue(
    ~~~~~~~~~~~~~~~^
        value == 0 and math.copysign(1, value) < 0,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        msg="Expected a negative zero, got {!r}".format(value)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
AssertionError: False is not true : Expected a negative zero, got 0.0

AMD64 Windows11 Bigmem PR

Unrelated failure: test_bigmem failed.

buildbot/iOS ARM64 Simulator PR

Unrelated failure:

1 test altered the execution environment (env changed):
    test_interpreters
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

Test failing on Android and FreeBSD:

        # Corner case where rounding the multiplication would
        # give the wrong result.
        x = float.fromhex('0x1p-500')
        y = float.fromhex('0x1p-550')
        z = float.fromhex('0x1p-1000')
        self.assertIsNegativeZero(math.fma(x-y, x+y, -z))
        self.assertIsPositiveZero(math.fma(y-x, x+y, z))
        self.assertIsNegativeZero(math.fma(y-x, -(x+y), -z))
        self.assertIsPositiveZero(math.fma(x-y, -(x+y), z))

z is not zero in this case.

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev

skirpichev commented Mar 12, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Proposed patch affects only case when the last argument (z) is zero, which is the case for most tests in this function. Maybe we can factor out other tests to a separate function (and then skip them).

Netbsd failure also seems related to z!=0 case. Not sure about WASI.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
serhiy-storchaka self-requested a review March 12, 2025 11:00
vstinner added 2 commits March 12, 2025 12:15
@vstinner
Add test_fma_zero_result2()
7dba022
@vstinner
Change coding style
4e7c5ab
@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

!buildbot wasi

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 4e7c5ab 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

The command will test the builders whose names match following regular expression: wasi

The builders matched are:

  • wasm32-wasi Non-Debug PR
  • wasm32-wasi PR
  • wasm32 WASI 8Core PR
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

!buildbot FreeBSD

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 4e7c5ab 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

The command will test the builders whose names match following regular expression: FreeBSD

The builders matched are:

  • AMD64 FreeBSD15 PR
  • AMD64 FreeBSD PR
  • AMD64 FreeBSD Refleaks PR
  • AMD64 FreeBSD14 PR
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

!buildbot Android

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 4e7c5ab 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

The command will test the builders whose names match following regular expression: Android

The builders matched are:

  • AMD64 Android PR
  • aarch64 Android PR
All reactions

Sorry, something went wrong.

Uh oh!

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

skirpichev
skirpichev reviewed Mar 12, 2025
View reviewed changes
Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 12, 2025
@bedevere-bot

bedevere-bot commented Mar 12, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 4e7c5ab 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131134%2Fmerge

If you want to schedule another build, you need to add the 🔨 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 Mar 12, 2025
@serhiy-storchaka

serhiy-storchaka commented Mar 12, 2025

Copy link
Copy Markdown
Member

It looks this PR does not fix the real issue. It only handles one specific case z == 0, but does not handle zillions more special cases when x * y is too close to -z. There may also be many cases when the result of fma() is not zero, but not precise. Handling this only for z == 0 can make all worse, because the function will no longer be monotonic.

To handle all cases, we need to implement fma() from scratch, using floating point with 106-bit or like mantissa. It was decided to not do it when this function was added.

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev

skirpichev commented Mar 13, 2025

Copy link
Copy Markdown
Member

It looks this PR does not fix the real issue.

Well, linked issue is a real one. Though, I expected more such cases. (Maybe WASI too?)

#131071 is an alternative. BTW, I think that test_fma_zero_result() could be split in that pr as well.

It only handles one specific case z == 0

That's true. Only simple workarounds were considered, not implementation of fma() from scratch.

There may also be many cases when the result of fma() is not zero, but not precise.

Maybe. But in this case our test suite lacks tests to trigger this. I think that at least failure with musl C stdlib may be related only to arguments with special values.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 13, 2025

Copy link
Copy Markdown
Member Author

@serhiy-storchaka is against this workaround: #131134 (comment). I abandon this approach.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner closed this Mar 13, 2025
@vstinner
vstinner deleted the fma_zero branch March 13, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skirpichev skirpichev skirpichev left review comments
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review

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.

4 participants

@vstinner @skirpichev @bedevere-bot @serhiy-storchaka
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.