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

Add KLU feature #6

Closed
wants to merge 7 commits into from

Conversation

martinjrobins
Copy link

@martinjrobins martinjrobins commented Mar 31, 2024

Fixes #3. This PR:

  • adds klu feature
  • adds SuiteSparse submodule (https://github.com/DrTimothyAldenDavis/SuiteSparse)
  • if klu feature is added, then bindings are added for the sundials KLU linear solver (requires both sundials and suitesparse to be installed)
  • If a vendor build, also builds and links suitesparse (using the SuiteSparse submodule) to the sundials library build (requires a BLAS installation to be present)

Note: I havn't added the klu feature to the CI yet. If would be good to get some feedback on how this should be tested. Should klu be a default feature, or should this be tested separately? Also, the current CI has an issue that I noticed: #5

build.rs Outdated Show resolved Hide resolved
@Chris00
Copy link
Owner

Chris00 commented Apr 20, 2024

Thanks for your PR. I builds fine with the system Sundials. I see two issues we need to resolve.

  • I think the paths for local build are not correct (cargo build --features klu fails) and one should act an extra environment variable for SuiteSparse — the default should be assuming it is installed alongside Sundials ($D/sundials/ and $D/suitesparse/) and not as a sub-directory. I can fix these but these paths may change due to the second issue.
  • SuiteSparse repository is 268Mb while a crate can at max be 10Mb — and most of the 10Mb are already taken by Sundials (stripped of its doc, examples,...). So suitesparse should be dynamically downloaded (say with gitoxide but an additional large download may be frowned upon, especially because it will increase the size of each target/) or one must require the user to first install SuiteSparse and point to its installation with an environment variable.

@Chris00
Copy link
Owner

Chris00 commented Apr 21, 2024

For the first point, I meant with the vendor libraries; cargo build --features klu,build_libraries to force it.

@martinjrobins
Copy link
Author

regarding the 2nd point: you're right, I didn't think about the size of the submodule. I'm happy with requiring the user to have a local installation of suitesparse and pointing to it via an environment variable. I think this is a reasonable requirement for an optional feature. I can make this change to the PR if you're also happy with this?

@Chris00
Copy link
Owner

Chris00 commented Apr 28, 2024

I pushed a commit enabling the KLU feature (in the dev branch). May you check that it works for you? If so, I'll make a new release.

@martinjrobins
Copy link
Author

The dev branch works perfectly for me, thanks @Chris00!

@Chris00
Copy link
Owner

Chris00 commented Apr 29, 2024

Thanks. I have released version 0.5.

@Chris00 Chris00 closed this Apr 29, 2024
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.

Sundials KLU linear solver
2 participants