BLD: Fix features.h detection and blocklist complex trig funcs on musl

[mbargull]

Fixes function blocklisting for glibc<2.18, reported in issue gh-25087.

closes gh-25087

[mbargull]

Could we add a test for the reproducer assert np.signbit(np.arctanh(-1j).real) from gh-25087 ?
Allow edits by maintainers is checked -- I'd appreciate it, if someone would just add a commit directly to this PR's branch for that.

[mbargull]

Thanks for adding the test, Matti!

[seberg]

Grrr, sounds like MUSL libc bugs again, which isn't surprising, I guess. Maybe should just xfail the test on MUSL for now?
But might be best to blocklist probably all complex trigonometric functions... A quick search I am not sure what would be an obvious MUSL detection macro.

[mattip]

[B]locklist probably all complex trigonometric functions.

Yes, makes sense

I am not sure what would be an obvious MUSL detection macro.

stack overflow says

#define _GNU_SOURCE
#include <features.h>
#ifndef __USE_GNU
    #define __MUSL__ 
#endif

I will try it

[mattip]

I added an #else branch for __GLIBC__. Maybe overkill to mark all the complex trig functions ...

[seberg]

Maybe overkill to mark all the complex trig functions ...

Yeah, not sure if we should start minimal, I doubt we can trust many of them but maybe some are good. Ideally we would see if some of them actually work, but it will probably also be version dependent.

I would hope that we have a few xfailed/skipped tests that actually start working with the blocklist.

[seberg]

I am OK with trying this, it looks a bit like we would block not just musl, but also other glibc-like implementations. It also may block too much, but... OTOH, all of that is probably very niche.

MUSL seems to insists on making detection hard/impossible. I am not sure if they want to make us run the code to know if their implementations are faulty, or whether they just never thought of the fact that functions might exist but be deficient.
(I honestly am not sure if we can run stuff at compile time. To me that would seem to at least kill any cross compilation attempt, which seems moot?)

[mattip]

CI passes on the last commit, so the mechanism must be working.

There is issue #23049, which has this list of failures:

2023-01-19T14:03:09.5394521Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_precisions_consistent - AssertionError: 
2023-01-19T14:03:09.5394788Z Arrays are not almost equal to 15 decimals fch-fcl <ufunc 'arctan'>
2023-01-19T14:03:09.5394889Z  ACTUAL: (1.0172219276428222656+0.40235948562622070312j)
2023-01-19T14:03:09.5394991Z  DESIRED: (1.0172219678978514+0.40235947810852507j)
2023-01-19T14:03:09.5395442Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_branch_cuts - AssertionError: (array([-1.57079633-0.54930614j, -1.57079633+0.54930614j]), array([1.57079633-0.54930614j, 1.57079633+0.54930614j]))
2023-01-19T14:03:09.5395957Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_branch_cuts_complex64 - AssertionError: (array([-1.5707963-0.54930615j, -1.5707963+0.54930615j], dtype=complex64), array([1.5707884-0.54930615j, 1.5707884+0.54930615j], dtype=complex64))
2023-01-19T14:03:09.5396333Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_loss_of_precision[complex64] - RuntimeWarning: divide by zero encountered in divide
2023-01-19T14:03:09.5396706Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_loss_of_precision[complex128] - RuntimeWarning: divide by zero encountered in divide
2023-01-19T14:03:09.5397075Z FAILED numpy/core/tests/test_umath.py::TestComplexFunctions::test_loss_of_precision[complex256] - RuntimeWarning: divide by zero encountered in divide

I unskipped them, tests passed locally, so I will mark this as
closes #23049

Edit: the issue linking is not working, when this gets merged please be sure to close that issue.

[mattip]

we would block not just musl, but also other glibc-like implementations

I think erring on the side of caution is preferable. If one of those glibc implementations get annoyed at us using a non-native routine (or demi quad float precision routines), they can suggest how to unblock the functions

[mbargull]

Yeah, replacing them all for musl is probably not too bad given:
https://wiki.musl-libc.org/open-issues.html#Complex_math

Currently most complex functions have dummy implementations. [...]

---

I one were to use them selectively one would have to do things like this:

if cc.run(
    '''
        #include <complex.h>
        #include <math.h>
        int main(void) {
            double complex x = -I;
            return signbit(creal(catanh(x))) ? 0 : 1;
        }
    ''',
    dependencies: m_dep,
).returncode() == 0
    # add HAVE_CATANH* defines
endif

which isn't too bad (those compilations/checks can be run very quickly).
But

• this is only one test case for one function (and there is much one could test),
• and as you mentioned, this would be pretty problematic for cross-compilations.

[mattip]

Hmm. More work is needed to blocklist frexp.

[mbargull]

Do we want the last 2 commits (testing more musl) cherry-picked for gh-25093 as well?

[mattip]

The usual backport workflow starts from a successful PR to main. So let's see what happens when the dust settles here.

[mattip]

There is a (new?) test failure on macos_arm64. Does macos (based on freebsd) pick up the blocklisting?

FAILED numpy/matrixlib/tests/test_matrix_linalg.py::TestQRMatrix::test_stacked_inputs[float32-outer_size2-size1] - RuntimeWarning: invalid value encountered in matmul

[mbargull]

There is a (new?) test failure on macos_arm64. Does macos (based on freebsd) pick up the blocklisting?

FAILED numpy/matrixlib/tests/test_matrix_linalg.py::TestQRMatrix::test_stacked_inputs[float32-outer_size2-size1] - RuntimeWarning: invalid value encountered in matmul

It should not (i.e., features.h would normally not be available on macOS, AFAIK).

[seberg]

There is a (new?) test failure on macos_arm64

I don't really remember it recently at least, is that with openblas or accelerate? In either case, it says in matmul... try rerunning, but it seems clearly unrelated.

(If it turns out non-flaky, I would ask if openblas version might have changed.)

[mattip]

CI is passing. Let's leave the fpexp blocklist for another day, it is not connected to complex trig and was there before #23049

[rgommers]

Looks good, thanks for working through this!

I one were to use them selectively one would have to do things like this:
if cc.run(

Please don't do that. @seberg is correct here:

(I honestly am not sure if we can run stuff at compile time. To me that would seem to at least kill any cross compilation attempt, which seems moot?)

We currently have a single place where we run non-native code (for detecting the long double binary format), and we added a way to specify the value of that format in a config file so the check could be bypassed for cross compilation. We should really avoid any usage of cc.run & co.

[mbargull]

Please don't do that. @seberg is correct here:

(I honestly am not sure if we can run stuff at compile time. To me that would seem to at least kill any cross compilation attempt, which seems moot?)

Definitely, yes.
I probably did not emphasize the "But ..."-part enough.
To paraphrase: IMO, there are too many corner-cases to properly assess the viability of those implementations during configuration and one can't reliably do that for cross-compilations (... and I value good cross-compiling support).

[charris]

I put the backport (#25093) in, it doesn't look quite the same. Does it need something more?

[mattip]

Does it need something more?

As long as CI is passing I think it is fine. The test changes that were not backported, as far as I can tell, remove some skips so the main branch should be more sensitive to new failures.

[mattip]

I think this is ready for review

[seberg]

Yes, it looks fine, just wanted to keep it briefly in case anyone has an opinion on the details of blocklisting.

I suppose backporting all of it makes sense, even if the blocklist might go a bit further than strictly necessary?

[charris]

As long as CI is passing I think it is fine

Only the test changes were missing and I'd rather not fool with those, so not going to worry about backporting this.