mreineck / pocketfft

Fork of https://gitlab.mpcdf.mpg.de/mtr/pocketfft to simplify external contributions
BSD 3-Clause "New" or "Revised" License
71 stars 34 forks source link

BUG: MAC version preprocessor shim #11

Closed tylerjereddy closed 5 months ago

tylerjereddy commented 5 months ago

"Upstream" version of https://github.com/scipy/pocketfft/pull/1 (@lucascolley suggested we at least flush the CI, regardless of where we merge it, if we merge it..)

lucascolley commented 5 months ago

I suppose there isn't CI on this repo... given that SciPy CI was passing before this change, I recommend we just try it and see if it fixes for the original issue authors

mreineck commented 5 months ago

Yes, CI is missing, since this repo has ben mostly dormant for several years. I'll see if I can add something (at least for compilation), but the trick will be to cover all the unusual OSs and OS versions that cause trouble.

That said, perhaps we should simply give up on the idea of a portable aligned_alloc and always emulate it. I'm doing this in ducc0 now, since the number of non-compliant systems simply makes this not wothwhile.

mreineck commented 5 months ago

Is this something that needs to be settled very urgently, or do we have time to try a version that unconditionally uses the home-grown aligned allocation?

lucascolley commented 5 months ago

Is this something that needs to be settled very urgently, or do we have time to try a version that unconditionally uses the home-grown aligned allocation?

It needs to be settled urgently for NumPy and SciPy, but we could apply the patch in the fork we maintain under the SciPy organisation, if you would like to try a more substantial fix in this repo.

EDIT: https://github.com/numpy/numpy/issues/25940 also reported an issue on Termux

mreineck commented 5 months ago

The (somewhat) more substantial patch I have in mind would be to simply remove the #if branch and always use the #else branch. That should be maximally future-proof.

But if you prefer to apply the proposed one-liner for the next numpy and scipy releases, that's perfectly fine for me as well. Then I'll just merge the PR, the repos will stay in sync, and we can think about more general solutions afterwads.

lucascolley commented 5 months ago

The (somewhat) more substantial patch I have in mind would be to simply remove the #if branch and always use the #else branch. That should be maximally future-proof.

This may be preferable, as I don't know whether anyone have found a patch which works for the Termux issue.

But if you prefer to apply the proposed one-liner for the next numpy and scipy releases, that's perfectly fine for me as well. Then I'll just merge the PR, the repos will stay in sync, and we can think about more general solutions afterwads.

This also sounds sensible, maybe no harm in merging this now then considering other options.

mreineck commented 5 months ago

Great, let's do it like this!

mreineck commented 5 months ago

Done! You can now choose between cb9988c86f4b005de5f77594026a8ba9efec45ee (the original PR) and 33ae5dc94c9cdc7f1c78346504a85de87cadaa12 (fully disabled aligned_alloc).