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 bindings to UMFPACK #327

Merged
merged 62 commits into from
Apr 23, 2023
Merged

Add bindings to UMFPACK #327

merged 62 commits into from
Apr 23, 2023

Conversation

jlogan03
Copy link
Collaborator

@jlogan03 jlogan03 commented Apr 17, 2023

  • Add build, raw bindings, and mid-level wrapper for i32-indexed and i64-indexed, real-valued versions of UMFPACK (the DI and DL variations)
    • Same footprint of bindings as scipy and Julia, I believe
  • Just covers the most-traveled parts (opaque LU factorization, solution of Ax=b problem type, and recovery of LU factorization components)
  • Unlike the other suitesparse integrations, this one only supports dynamic linking
    • The build for UMFPACK depends on several of the other packages plus BLAS, and while it's definitely possible to do it, it looks extremely time-consuming and I think it's best to leave that for another time

…ngs to use platform-dependent compatible integer type.
…meric factorizations to build context struct.
@jlogan03
Copy link
Collaborator Author

Alrighty, ready for another pass of CI & passing lint/test locally

@jlogan03
Copy link
Collaborator Author

Resolved the latest round of errors & ready for another go

Copy link
Collaborator

@mulimoen mulimoen left a comment

Choose a reason for hiding this comment

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

This is some great work! Just a couple of minor nits and this should be good to go.

I'll be happy to have a look at the static building later on. It should be possible to build this using e.g. cargo check --features suitesparse_ldl_sys/static, we can add a feature flag to more crates to make this easier to enable

@jlogan03
Copy link
Collaborator Author

This is some great work! Just a couple of minor nits and this should be good to go.

I'll be happy to have a look at the static building later on. It should be possible to build this using e.g. cargo check --features suitesparse_ldl_sys/static, we can add a feature flag to more crates to make this easier to enable

Sounds good - I'll be interested to follow along and might give it another try myself eventually

@jlogan03
Copy link
Collaborator Author

Good to go for another round of CI - might even run to completion this time...

@jlogan03
Copy link
Collaborator Author

So, I'm not able to reproduce this CAMD test failure locally (on a mac), but my best guess is that it might be a semver-non-compliant change in cc, so I rolled the patch version for suitesparse-src's cc dep back to what it was before. Other than that, I don't think anything I've changed here should have an effect.

@jlogan03
Copy link
Collaborator Author

For the first failed job, that looks like a network issue.

The second one (the failing camd tests on mac) are the same as before, and I have no idea why it's happening. I'm able to run those tests locally on mac without issue:

test result: ok. 28 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.39s

(py310) Jamess-MacBook-Pro:sprs_suitesparse_camd jlogan$ cargo test --features suitesparse_camd_sys/static -p sprs

@jlogan03
Copy link
Collaborator Author

Ok, on further inspection, I realized that test is running on nightly - I do get that error locally running on the nightly channel, but not on stable. Seems to be a bug in nightly unrelated to the changes on this PR

@mulimoen
Copy link
Collaborator

I find the random CI error strange. I reran CI from master which gives no errors: https://github.com/sparsemat/sprs/actions/runs/4705082629

I am not sure if this is a miscompilation or a bug in some code we have here.

@jlogan03
Copy link
Collaborator Author

I find the random CI error strange. I reran CI from master which gives no errors: https://github.com/sparsemat/sprs/actions/runs/4705082629

I am not sure if this is a miscompilation or a bug in some code we have here.

This is running successfully on nightly on my mac now - looks like whatever was busted on nightly, they must have fixed it overnight

@jlogan03
Copy link
Collaborator Author

...aaaand it's breaking again. I guess since it's a pointer alignment issue, it will probably be intermittent

@jlogan03
Copy link
Collaborator Author

...and running again - definitely intermittent

@jlogan03
Copy link
Collaborator Author

The benign changes to suitesparse-src that were leftover from reversing my attempt to do the static build have been removed just in case those are somehow the problem, and this is ready for another CI attempt

@jlogan03
Copy link
Collaborator Author

Figured out how to get rid of all those trailing underscores & I believe this should be ready to merge

@mulimoen mulimoen merged commit 9711333 into sparsemat:master Apr 23, 2023
@mulimoen
Copy link
Collaborator

Thanks a lot for your work in this @jlogan03!

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.

2 participants