8000 BUG: Refactor complex floor_divide to avoid osx-arm64 opt warnings by erykoff · Pull Request #17712 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

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
@isuruf isuruf mentioned this pull request Mar 7, 2021
Merged
@ogrisel
Copy link
Contributor
ogrisel commented Mar 7, 2021

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

@erykoff erykoff mentioned this pull request Mar 8, 2021
Closed
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@erykoff @charris @ogrisel @rgommers @isuruf @mattip
0