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 qdldl lin sys solver #211

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

zach401
Copy link

@zach401 zach401 commented Jan 18, 2022

Replaced LDL library with QDLDL which is available under an Apache license.

The goal of this change is to remove a dependency on GPL licensed code.

For the most part this was a one-to-one replacement.
There were a few breaking changes, however.

  • QDLDL_factor does not support fine grained profiling, so I have removed tfactor_t1 and trfactor_t2 which stored the time spend computing the sparsity pattern and time spend on the numerical solve respectively.
  • The long type has been updated to long long instead of long for consistency with QDLDL. If we prefer, we could update QDLDL to use long instead.
  • QDLDL_factor does not support eps or delta arguments, so they have been removed.
  • There is a slight reduction in precision in test/exponential/log_ax_x.h, while the original test required an absolute error of<1e-11 the new linear system solver produced an absolute error <1.3e-11.

Open Questions:

  • I am by no means an expert in Makefile and cmake, so there may be preferable ways to structure these.
  • Do we want to use a submodule for QDLDL like OSQP, or copy the repository directly, like LDL was previously?
  • Is there a way to build qdldl directly into the ecos library .so and .a files? This way we won't have to change the build and installation process for ecos by having users install libqdldl.so.

This change is Reviewable

@adomahidi
Copy link
Member

Thanks @zach401 for taking a stab at this, very much appreciated.

Some input from my side regarding your questions:

  • the necessity of removing eps and delta parameters hints at QDLDL not having dynamic regularization. We'd need to investigate the effect of not having this; in certain numerically challenging situations, this could help robustness. It's not hard to introduce, but would need adapting (a version of) QDLDL.
  • submodules are a pain to work with but on the other hand would allow locking ecos against certain versions (commits) of QDLDL, which could be updated if updates become available. If we need (for the point mentioned above) an "ecos-adapted" version of QDLDL here, then it would make more sense to copy it over and integrate into the repo. Makes the build also much easier :)
  • Regarding your last point, shouldn't it be the same/similar as with SparseLDL? Could adapt the Makefiles to build the different modules and then link everything together to an .a or .so file

@adomahidi
Copy link
Member

Ah and one more thing, any idea why the Appveyor build is failing?

@imciner2
Copy link

Do we want to use a submodule for QDLDL like OSQP, or copy the repository directly, like LDL was previously?

I'd suggest just copying the files for QDLDL into ECOS directly and making a note of which version they are so that you can keep track of if they need to be updated in the future. We decided in OSQP to actually move away from the submodule for QDLDL in the upcoming 1.0 release because it had been a major source of pain for users who just wanted to clone the repo or download a GitHub tarball of the source code (since the default clone is not recursive, and the GitHub-generated tarball doesn't recurse into submodules), so we have had to end up producing our own tarball for users who want the complete code for the latest releases.

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