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: Fixes for ARM architecture #10763

Merged
merged 17 commits into from
Jun 17, 2022
Merged

BUG: Fixes for ARM architecture #10763

merged 17 commits into from
Jun 17, 2022

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jun 14, 2022

Bad things found so far on my M1 machine:

  • We can't rely on LinAlgError to trigger on np.linalg.solve for rank deficiency on arm64. I changed the mxne code to use a SVD, which actually seems faster (!?) so I think this is a tolerable change at least. This probably deserves an upstream bug report at some point, but isolating it isn't the easiest thing...
  • We can't rely on SVD: BUG: linalg.svd broken on M1 / arm64 numpy/numpy#21756

The second one is a show stopper. I don't think we can safely release or advocate for M1-based installers until it's fixed.

There are a bunch of test_linalg.py errors that I see locally that are likely related, but I don't want to bother looking into them until we can trust svd.

Helps with #10759

@larsoner
Copy link
Member Author

@hoechenberger or @agramfort if either of you can replicate numpy/numpy#21756 locally and can comment on the NumPy issue, it might help move things forward -- and also make sure I'm not doing something wrong!

@hoechenberger
Copy link
Member

OMG this is horrible 😱 😱

@hoechenberger
Copy link
Member

Seems to be a conda-forge problem, we should open an issue with them

numpy/numpy#21756 (comment)

@hoechenberger
Copy link
Member

@larsoner Shall we briefly catch up on Discord?

@hoechenberger
Copy link
Member

For all the others following this thread, the problem seems to be related to OpenBLAS, and switching to a different BLAS implementation seems to fix at least this here test case.

@larsoner
Copy link
Member Author

Sorry I was fighting with https://gitlab.kitware.com/vtk/vtk/-/issues/18559 for the last hour instead :)

I think the most likely scenario is that it's an OpenBLAS issue that probably needs to be fixed at the OpenBLAS end. But now that it has been replicated on two machines, the NumPy folks should know how to move things forward. So I think we wait a couple of days and see if they make progress. In the meantime I think we wait on the M1 installers :(

@hoechenberger
Copy link
Member

hoechenberger commented Jun 14, 2022

Can you try to conditionally use accelerate on ARM64 only, which is Apple's own implementation of BLAS?

@larsoner
Copy link
Member Author

Can you try to conditionally use accelerate on ARM64 only, which is Apple's own implementation of BLAS?

I guess we could try this, but IIRC Accelerate was not really well supported by SciPy last time I saw it discussed. Feel free to try locally and run make pytest to see what fails. I'll give it a shot tomorrow.

@larsoner larsoner marked this pull request as ready for review June 15, 2022 03:14
@larsoner
Copy link
Member Author

@agramfort feel free to merge if you're okay with the mxne changes

@larsoner larsoner added this to the 1.1 milestone Jun 15, 2022
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nitpick. than +1 for MRG on my side

thx @larsoner

mne/inverse_sparse/mxne_optim.py Show resolved Hide resolved
@hoechenberger
Copy link
Member

Great work, @larsoner!

@larsoner larsoner deleted the arm branch June 17, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants