BUG: sparse.linalg: Update SuperLU to fix unfilterable output for singular matrices

[dschmitz89]

Reference issue

Closes gh-20993

What does this implement/fix?

Updates SuperLU to the head of their master branch. A recent commit should avoid printing to stdout for singular matrices.

Additional information

Locally, on WSL using Ubuntu and conda compilers, compilation fails due to a recent SuperLU update regarding complex numbers.

Edit: compilation also fails in CI. After some feedback, I will open an issue upstream.

[dschmitz89]

Using the second latest commit from SuperLU also results in another compilation failure 🙈 :

[129/188] Compiling C object scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_sp_preorder.c.o
FAILED: scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_sp_preorder.c.o
/home/tyrion/miniforge3/envs/scipy-dev/bin/x86_64-conda-linux-gnu-cc -Iscipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p -Iscipy/sparse/linalg/_dsolve -I../scipy/sparse/linalg/_dsolve -I../scipy/sparse/linalg/_dsolve/SuperLU/SRC -fvisibility=hidden -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c17 -O2 -g -Wno-unused-but-set-variable -Wno-unused-function -Wno-conversion -Wno-misleading-indentation -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/tyrion/miniforge3/envs/scipy-dev/include -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem /home/tyrion/miniforge3/envs/scipy-dev/include -fPIC -DUSE_VENDOR_BLAS=1 -Wno-unused-variable -Wno-parentheses -Wno-unused-label -Wno-implicit-function-declaration -Wno-switch -Wno-unused-result -Wno-maybe-uninitialized -MD -MQ scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_sp_preorder.c.o -MF scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_sp_preorder.c.o.d -o scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_sp_preorder.c.o -c ../scipy/sparse/linalg/_dsolve/SuperLU/SRC/sp_preorder.c
../scipy/sparse/linalg/_dsolve/SuperLU/SRC/sp_preorder.c: In function 'sp_preorder':
../scipy/sparse/linalg/_dsolve/SuperLU/SRC/sp_preorder.c:81:17: error: conflicting types for 'check_perm'; have 'void(char *, int,  int *)'
   81 |     extern void check_perm(char *what, int n, int *perm);
      |                 ^~~~~~~~~~
In file included from ../scipy/sparse/linalg/_dsolve/SuperLU/SRC/slu_ddefs.h:95,
                 from ../scipy/sparse/linalg/_dsolve/SuperLU/SRC/sp_preorder.c:16:
../scipy/sparse/linalg/_dsolve/SuperLU/SRC/slu_util.h:419:16: note: previous declaration of 'check_perm' with type 'int(char *, int,  int *)'
  419 | extern int     check_perm(char *what, int n, int *perm);
      |                ^~~~~~~~~~
[134/188] Compiling C object scipy/sparse/linalg/_dsolve/libsuperlu_lib.a.p/SuperLU_SRC_smemory.c.o
ninja: build stopped: subcommand failed.
Build failed!

Guess we have to bite the bullet and get this fixed upstream then.

[dschmitz89]

Locally things are working now, just need to properly recreate the patch file, messed that one up badly.

[dschmitz89]

CC @nickodell would you have time to review?

[nickodell]

CC @nickodell would you have time to review?

Hi, I've verified two things.

• The code compiles for me.
• The reproducer from BUG: spsolve prints "dgstrf info" to stdout on singular matrices #20993 is fixed.

What other things should I check?

[dschmitz89]

CC @nickodell would you have time to review?

Hi, I've verified two things.

• The code compiles for me.
• The reproducer from BUG: spsolve prints "dgstrf info" to stdout on singular matrices #20993 is fixed.

What other things should I check?

Thanks! What I am not quite sure about are the patches I added on top of SuperLU: https://github.com/scipy/scipy/blob/629e358b943818cbeac9fd26b0e79f0bd8655020/scipy/sparse/linalg/_dsolve/SuperLU/scipychanges.patch

Most of them are for singular matrix handling and our tests pass though.

[dschmitz89]

Note that if this gets merged and backported to 1.14.1, it also comes with the SuperLU license fix, so we would not need to double backport.

[rgommers]

What I am not quite sure about are the patches I added on top of SuperLU:

It would probably be useful to make a difference here between what we were already patching, and the new patches you added. Perhaps comment on each of those and why they need to be kept as SciPy patches rather than upstreamed?

[nickodell]

Thanks! What I am not quite sure about are the patches I added on top of SuperLU: https://github.com/scipy/scipy/blob/629e358b943818cbeac9fd26b0e79f0bd8655020/scipy/sparse/linalg/_dsolve/SuperLU/scipychanges.patch

Many of these changes to the patch file seem unrelated to what's being fixed. Can you explain why the patch file is changing at all? We're updating the file being patched - we shouldn't need to change the patch file, right?

It might help if you explained the process here for how the SuperLU update is being done. I'm guessing you're diffing our modified SuperLU against the old upstream unmodified SuperLU to generate the patch, then applying the patch to the upstream updated SuperLU to generate a modified version. Is that right?

[dschmitz89]

Commenting this out is necessary. Otherwise, at runtime we get errors about missing METIS symbols.

[rgommers]

This change seems fine. Enabling this unconditionally is probably not a good thing to do upstream (that's what build system checks are for), but let's not worry about it too much for now since the patch is harmless.

[dschmitz89]

I thought the same. Unfortunately, setting it to FALSE is also not enough. Commented upstream here: xiaoyeli/superlu@6f7ea89#r145014670

[dschmitz89]

Thanks! What I am not quite sure about are the patches I added on top of SuperLU: https://github.com/scipy/scipy/blob/629e358b943818cbeac9fd26b0e79f0bd8655020/scipy/sparse/linalg/_dsolve/SuperLU/scipychanges.patch

Many of these changes to the patch file seem unrelated to what's being fixed. Can you explain why the patch file is changing at all? We're updating the file being patched - we shouldn't need to change the patch file, right?

It might help if you explained the process here for how the SuperLU update is being done. I'm guessing you're diffing our modified SuperLU against the old upstream unmodified SuperLU to generate the patch, then applying the patch to the upstream updated SuperLU to generate a modified version. Is that right?

The patch is against the used SuperLU source files. The only new patch is commenting out METIS stuff, see my last comment here. As for the other patches, I tried to stay faithful to what we had before but had to juggle with more if. CC @rgommers

[dschmitz89]

One more thing that came to mind is that we should probably better document which commit of SuperLU we used when the code was copied. Looking through PR comments can be tedious. So far we probably only ever used releases, so this was not an issue.

[rgommers]

Thanks @dschmitz89. Adding the exact commit we're pointing at sounds good. I suggest adding a README.scipy file, mirroring what we do for ARPACK and QHull.

[dschmitz89]

This is ready to merge from my side, CI failures are unrelated.

[nickodell]

LGTM. I'm not sure I understand this code well enough to spot potential problems, but I don't see any issues.

[rgommers]

The file mc64ad.c was incorrectly added. Can you please remove it again, squash the history so it is really gone from all commits, and add to README.scipy the procedure for keeping it out?

That file isn't license-compatible. See gh-12243 for a previous discussion on this.

[rgommers]

The CI failures are because an unintentional update of the array_api_compat git submodule snuck in here.

[dschmitz89]

Argh, will try to fix after work. I wish there was a pre commit that warns for submodule changes.

[dschmitz89]

Submodule update reverted. Commits should still be squashed as they are not clean.

[rgommers]

LGTM now, thanks a lot @dschmitz89! Time to give this a go:) And thanks for the review @nickodell

[rgommers]

@tylerjereddy I'll leave the backport label for you to decide. It seems okay to do if it cherry-picks cleanly, but it's a large diff and the bug this fixes isn't super critical, so leaving it out would also be reasonable I think.

[tylerjereddy]

The cherry-pick was clean and passed tests locally on ARM mac so I've tentatively added it to the backport PR, thanks