Skip to content

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

Closed
erykoff wants to merge 1 commit into
numpy:mainfrom
erykoff:complexfloordiv
Closed

erykoff wants to merge 1 commit into
numpy:mainfrom
erykoff:complexfloordiv

Conversation

@erykoff

@erykoff erykoff commented Nov 5, 2020

Copy link
Copy Markdown

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

charris commented Nov 5, 2020

Copy link
Copy Markdown
Member

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

@charris

charris commented Nov 5, 2020

Copy link
Copy Markdown
Member

The test failure looks real.

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

ogrisel commented Mar 7, 2021

Copy link
Copy Markdown
Contributor

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

@rgommers

Copy link
Copy Markdown
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

isuruf commented Mar 13, 2021

Copy link
Copy Markdown
Contributor

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

@rgommers

Copy link
Copy Markdown
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
Copy Markdown
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

mattip commented Mar 17, 2021

Copy link
Copy Markdown
Member

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.

6 participants