Skip to content

gh-131032: Fix math.fma(x, y, z) zero sign #131134

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 12, 2025

Fix result sign when z is zero.

Co-Authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@skirpichev skirpichev self-requested a review March 12, 2025 07:46
@skirpichev
Copy link
Member

!buildbot wasi

@bedevere-bot
Copy link

🤖 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

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 12, 2025
@vstinner
Copy link
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.

@vstinner
Copy link
Member Author

cc @serhiy-storchaka

@vstinner
Copy link
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

@vstinner
Copy link
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.

@skirpichev
Copy link
Member

skirpichev commented Mar 12, 2025

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.

@serhiy-storchaka serhiy-storchaka self-requested a review March 12, 2025 11:00
@vstinner
Copy link
Member Author

!buildbot wasi

@bedevere-bot
Copy link

🤖 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

@vstinner
Copy link
Member Author

!buildbot FreeBSD

@bedevere-bot
Copy link

🤖 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

@vstinner
Copy link
Member Author

!buildbot Android

@bedevere-bot
Copy link

🤖 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

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 12, 2025
@serhiy-storchaka
Copy link
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.

@skirpichev
Copy link
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.

@vstinner
Copy link
Member Author

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

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants