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

Make building with UMFPACK optional. #579

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Conversation

mmuetzel
Copy link
Contributor

Add new CMake variable WITH_UMFPACK which can be used to build without using any functions from the UMFPACK library.

The default for this new variable is TRUE so there is no difference in the build configuration to before unless a user explicitly configures with, e.g., -DWITH_UMFPACK=OFF.

This probably fixes the issue described in #578.

With these changes, some tests are failing when built with WITH_UMFPACK=OFF with an error like the following in the logs:

ERROR:: GetMatrixFormat: UMFPACK solver has not been installed.

Skip all tests conditional on HAVE_UMFPACK for which the string "UMFPACK solver has not been installed." was found in the logs.

I might have missed some tests that should be skipped but weren't run for me (because they depend on other features with which I might not have built).

@raback
Copy link
Contributor

raback commented Sep 23, 2024

I was thinking whether we should modify the code such that the many tests are run even when "umfpack" is not found. After all, direct solvers should give the same results. Maybe there could be a warning if "umfpack" not found and using "mumps" instead. Maybe this is overthingking, since probably cases when "umfpack" is not compiled are rare (never happened before this...).

@mmuetzel
Copy link
Contributor Author

Yes, you are right. The UMFPACK backend was available in any case before the changes proposed here. (Either the bundled implementation or libraries installed on the system.)
If I understood #578 correctly, it could be desirable to build Elmer without UMFPACK in some situations. Imho, it would be best to also adapt the test suite in case UMFPACK should be made an optional feature for ElmerFEM.

I don't know how to implement an alternative backend for the affected solvers.
But even if that were to be implemented, the affected tests should probably be guarded with IF(HAVE_UMFPACK OR HAVE_MUMPS) instead of the guard on HAVE_UMFPACK (only) that is proposed in this PR. After all, Mumps is optional already (and probably should be kept optional).
So, adding a guard around these tests would probably be necessary in either case.

The guards that are added in this PR could still be modified in the future in case the alternative Mumps backend for these solvers should be implemented.

@raback
Copy link
Contributor

raback commented Sep 24, 2024

I took your 1st commit without the change in the tests and added a few lines of code in branch "umfpack2" where umfpack is changed to MUMPS, if available (with a Warning). I would think that either has to be present. Direct solvers are a backbone of robustness. So this would save the several hundreds tests. Even previously umfpack is switched to some parallel solvers if trying to use it in parallel so we already did that compromise. (This actually didn't compile on my VM with WITH_UMFPACK=FALSE).

@mmuetzel
Copy link
Contributor Author

Thank you for showing the necessary changes to fall back from UMFPACK to Mumps as the direct solver.

It would still be possible for a user to configure without UMFPACK and without Mumps at the same time. I don't know if there are edge cases where this would still be useful? Should the configuration emit a big fat warning if a user does that?

It will probably take a day or two before I can start looking into this again in more detail. I'll mark this PR as a draft for the time being.

@mmuetzel mmuetzel marked this pull request as draft September 25, 2024 07:25
mmuetzel and others added 5 commits September 26, 2024 16:40
Add new CMake variable `WITH_UMFPACK` which can be used to build without
using any functions from the UMFPACK library.

The default for this new variable is `TRUE` so there is no difference in
the build configuration to before unless a user explicitly configures
with, e.g., `-DWITH_UMFPACK=OFF`.

This probably fixes the issue described in ElmerCSC#578.
Some tests are failing when built with `WITH_UMFPACK=OFF` and `WITH_MUMPS=OFF`
with an error like the following in the logs:
```
ERROR:: GetMatrixFormat: UMFPACK (and MUMPS) solver has not been installed.
```

Skip all tests conditional on `HAVE_UMFPACK OR HAVE_MUMPS` for which the string
"UMFPACK (and MUMPS) solver has not been installed." was found in the logs.
@mmuetzel
Copy link
Contributor Author

I took the liberty to include your changes in this PR.

Marking as ready for review again.

@mmuetzel mmuetzel marked this pull request as ready for review September 26, 2024 15:34
@mmuetzel
Copy link
Contributor Author

I tested without UMFPACK but with MUMPS and without either UMFPACK and MUMPS. Configuration and building succeeds and all tests (with the quick label) pass for me in these configurations.
In the latter configuration a prominent warning is shown close to the end of the configuration (and many tests that require UMFPACK or MUMPS are skipped as expected).

@raback
Copy link
Contributor

raback commented Sep 27, 2024

Thanx Markus! This testing is now perfect since also without umfpack we have good coverage.

@raback raback merged commit c290533 into ElmerCSC:devel Sep 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants