Skip to content
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

BUG: Refactor complex floor_divide to avoid osx-arm64 opt warnings #17712

Closed
wants to merge 1 commit into from

Conversation

erykoff
Copy link

@erykoff erykoff commented Nov 5, 2020

When building numpy on the new Apple Silicon osx-arm64 platform using clang11 (see https://conda-forge.org/blog/posts/2020-10-29-macos-arm64/ for details), numpy fails tests because of extra divide-by-zero warnings when performing floor division with complex number arrays, even though the computations themselves work just fine.

I tracked this to the code in @TYPE@_floor_divide for complex types, where it appears that the optimizer on arm64 ends up dispatching both branches at the same time. Even though the second branch is discarded because it is not the code path, the divide-by-zero leaves behind a "ghost" floating point exception, and the tests fail.

Replacing the code with the same routine that is used for scalar complex floor division in scalarmath.c (using npy_divmod) does not suffer from this problem. It is also reasonable, I believe, for scalar and array methods to use the same algorithm.

@charris
Copy link
Member

charris commented Nov 5, 2020

This sounds like a clang bug that should also be reported upstream.

@charris
Copy link
Member

charris commented Nov 5, 2020

The test failure looks real.

Base automatically changed from master to main March 4, 2021 02:05
@ogrisel
Copy link
Contributor

ogrisel commented Mar 7, 2021

There is a similar issue for float32 / float64 power computation: gh-18555.

@rgommers
Copy link
Member

It is also reasonable, I believe, for scalar and array methods to use the same algorithm.

That does sound reasonable. Seems like this didn't pass CI though, rebased to get a fresh signal.

@isuruf
Copy link
Contributor

isuruf commented Mar 13, 2021

#18571 is an alternative to this as the tests don't pass with this PR

@rgommers
Copy link
Member

This does still fail:

___________________ TestDivision.test_floor_division_complex ___________________

self = <numpy.core.tests.test_umath.TestDivision object at 0x7fda6e16e090>

    def test_floor_division_complex(self):
        # check that implementation is correct
        msg = "Complex floor division implementation check"
        x = np.array([.9 + 1j, -.1 + 1j, .9 + .5*1j, .9 + 2.*1j], dtype=np.complex128)
        y = np.array([0., -1., 0., 0.], dtype=np.complex128)
        assert_equal(np.floor_divide(x**2, x), y, err_msg=msg)
        # check overflow, underflow
        msg = "Complex floor division overflow/underflow check"
        x = np.array([1.e+110, 1.e-110], dtype=np.complex128)
>       y = np.floor_divide(x**2, x)
E       RuntimeWarning: overflow encountered in floor_divide

msg        = 'Complex floor division overflow/underflow check'
self       = <numpy.core.tests.test_umath.TestDivision object at 0x7fda6e16e090>
x          = array([1.e+110+0.j, 1.e-110+0.j])
y          = array([ 0.+0.j, -1.+0.j,  0.+0.j,  0.+0.j])

The alternative in gh-18571 is easier to verify, and currently passing. So maybe we should prefer that. Anyone have an opinion?

@rgommers
Copy link
Member

#18571 is an alternative to this as the tests don't pass with this PR

Yep, looking at gh-18571 is how I got here:)

@mattip
Copy link
Member

mattip commented Mar 17, 2021

The alternative in #18571 was merged. Closing, please reopen or resubmit if there is more to do here.

@mattip mattip closed this Mar 17, 2021
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.

None yet

6 participants