Restore compatibility with osqp version 0.6.0 #32

GiulioRomualdi · 2019-09-01T00:46:14Z

This PR restore the compatibility with osqp version 0.6.0.
When this PR is merged, it will be not possible to use osqp 0.5.0 along with osqp-eigen. Indeed osqp/osqp@78d1135 changes osqp_setup() interface. This problem should be taken into account since a custom version of osqp may be used by internal projects (e.g. https://github.com/robotology-dependencies/osqp/tree/fix-lf-problems)

See #31

S-Dafarra · 2019-09-01T00:52:01Z

include/OsqpEigen/Data.tpp

@@ -32,7 +32,9 @@ bool OsqpEigen::Data::setHessianMatrix(const Eigen::SparseMatrix<T> &hessianMatr
    }

    //set the hessian matrix
-    if(!OsqpEigen::SparseMatrixHelper::createOsqpSparseMatrix(hessianMatrix, m_data->P)){
+    // osqp 0.6.0 required only the upper triangular part of the hessian matrix
+    Eigen::SparseMatrix<T> hessianMatrixUpperTriangular = hessianMatrix.template triangularView<Eigen::Upper>();


Is this method allocating memory?

Good point. Could we move this to be a buffer in the class, or we can't because the method is templated?

I think it allocates memory

I would suggest to keep a copy of the hessian and iterate over the upper triangular non-zeros of the input hessian. In case an element is already present you can copy the value, otherwise you add it. The problem is if the saved hessian has more non-zeros than the input hessian.

This function is called only the first time.
Probably there is a way to avoid to allocate memory

In which sense "only the first time"? It is not called every time the hessian changes?

No, the function called every time the hessian changes is

osqp-eigen/include/OsqpEigen/Solver.tpp

Line 13 in bbd96ea

bool OsqpEigen::Solver::updateHessianMatrix(const Eigen::SparseCompressedBase<Derived> &hessianMatrix)

Ah, sorry. Then it makes sense.

I thought as well it was called every time. So there are no problems I guess.

Unfortunately, it is not easy to extract the upper triangular matrix from a CSC representation (representation used by osqp). Furthermore, the methods used to retrieve the outerIndexPtr and the innerIndexPtr are defined in Eigen::SparseCompressedBase and the
Eigen::TriangularView class does not inherit from Eigen::SparseCompressedBase.

This is the reason why the following line is needed

Eigen::SparseMatrix<T> hessianMatrixUpperTriangular = hessianMatrix.template triangularView<Eigen::Upper>();

traversaro · 2019-09-01T10:52:31Z

Some small suggestions (not directly related to this PR, to be honest):

Could you tag and release the last commit before the PR? It will be the last one compatible with osqp 0.5, and it contains a single commit, but quite important w.r.t. to relese 0.3.0 .
I would increase the CMake version, either in this PR or in a later commit.
Do you think it could make sense to mantain a table in the docs to document the relative compatibility between osqp and osqp-eigen version? Something like:

osqp-eigen version	osqp version
0.3	Compatible with osqp <= 0.5
0.4	Compatible with osqp >= 0.6

GiulioRomualdi · 2019-09-09T17:05:07Z

cc @traversaro

Some small suggestions (not directly related to this PR, to be honest):

Could you tag and release the last commit before the PR? It will be the last one compatible with osqp 0.5, and it contains a single commit, but quite important w.r.t. to relese 0.3.0 .

I would increase the CMake version, either in this PR or in a later commit.

Do you think it could make sense to mantain a table in the docs to document the relative compatibility between osqp and osqp-eigen version? Something like:

osqp-eigen version osqp version
0.3 Compatible with osqp <= 0.5
0.4 Compatible with osqp >= 0.6

Actually 0.3 is the current version of master while 0.4 is the version in devel. Probably before merging the PR I should merge devel into master
By the way, as far as I understood is not possible to check the installed version of osqp from CMake

traversaro · 2019-09-09T19:47:10Z

By the way, as far as I understood is not possible to check the installed version of osqp from CMake

Upstream issue on this: osqp/osqp#195 .

Actually 0.3 is the current version of master while 0.4 is the version in devel. Probably before merging the PR I should merge devel into master

Mhh, good to know. independentily on what 0.3 or 0.4 are compatible with, such as table would help. Anyhow, considering that devel branch was never released, you can also merge the 0.6-compatibility in master, and then forward merge to devel.

GiulioRomualdi · 2019-09-20T12:47:28Z

Mhh, good to know. independentily on what 0.3 or 0.4 are compatible with, such as table would help. Anyhow, considering that devel branch was never released, you can also merge the 0.6-compatibility in master, and then forward merge to devel.

I don't know if this is a good idea. Right now the devel branch ensures back compatibility with master. In details, devel deprecate some functions defined in master.

On the other hand, when this PR will be merged the back-compatibility with a previous version of osqp will be broken. And I'm afraid that it will create problems -- see https://github.com/robotology-dependencies/osqp/tree/fix-lf-problems.

So to overcome possible issues, I would suggest to:

Merge devel into master
Merge this PR in devel (And probably change the version number from 0.4 to 0.4.1)
Because of Export osqp version with CMake configuration files osqp/osqp#195, it is not possible to check the version of osqp directly from CMake. Thus I will update the README.md describing the current status of osqp-eigen
As soon as Use latest tagged version of upstream osqp and osqp-eigen instead of local fork robotology-superbuild#242 will be fixed, devel will be merged into master

What do you think @traversaro @S-Dafarra?

traversaro · 2019-09-20T13:39:30Z

As soon as robotology/robotology-superbuild#242 will be fixed, devel will be merged into master

We can fix immediately the version of osqp-eigen in the superbuild, so that you are free to work as you prefer, and we update the osqp and osqp-eigen version when dust has setted .

traversaro · 2019-09-20T13:44:03Z

Merge this PR in devel (And probably change the version number from 0.4 to 0.4.1)

To be honest, I think that if the required version of osqp changes, it is better to at least bump the minor version.

GiulioRomualdi · 2019-09-23T11:02:39Z

I'm updating the version from 0.4.1 to 0.5.0, at the same time I would like also to change the compatibility from AnyNewerVersion to SameMinorVersion

 include(InstallBasicPackageFiles)
 install_basic_package_files(${PROJECT_NAME}
   VERSION ${${PROJECT_NAME}_VERSION}
-  COMPATIBILITY AnyNewerVersion
+  COMPATIBILITY SameMinorVersion
   VARS_PREFIX ${PROJECT_NAME}
   NO_CHECK_REQUIRED_COMPONENTS_MACRO
   CONFIG_TEMPLATE ${CMAKE_SOURCE_DIR}/cmake/OsqpEigenConfig.cmake.in)

Unfortunately, this option is available only from cmake v3.11 and Ubuntu 18.04 supports at most cmake v3.10. @traversaro, do you know if there is a workaround that can be used?

traversaro · 2019-09-23T11:22:38Z

@traversaro, do you know if there is a workaround that can be used?

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

traversaro · 2019-09-23T11:52:40Z

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

Note that you will need to copy just the BasicConfigVersion-***.cmake.in that you use, and you need to modify the ${CMAKE_ROOT} in https://github.com/Kitware/CMake/blob/master/Modules/WriteBasicConfigVersionFile.cmake#L36 to ${CMAKE_CURRENT_LIST_DIR}.

- upload a custom version of BasicConfigVersion-SameMinorVersion.cmake.in and WriteBasicConfigVersionFile.cmake to guarantee compatibility with cmake < 3.11 - compatibility ensured only if the required version is 0.5.*

GiulioRomualdi · 2019-09-23T14:05:19Z

@traversaro

You can just vendor WriteBasicConfigVersionFile and its included files BasicConfigVersion-***.cmake.in in your project, it should work fine (I guess).

Note that you will need to copy just the BasicConfigVersion-***.cmake.in that you use, and you need to modify the ${CMAKE_ROOT} in https://github.com/Kitware/CMake/blob/master/Modules/WriteBasicConfigVersionFile.cmake#L36 to ${CMAKE_CURRENT_LIST_DIR}.

Done in ac2a551

GiulioRomualdi · 2019-09-23T16:33:17Z

This PR is currently blocked by robotology/robotology-superbuild#260

GiulioRomualdi requested review from traversaro and S-Dafarra September 1, 2019 00:46

GiulioRomualdi self-assigned this Sep 1, 2019

S-Dafarra reviewed Sep 1, 2019

View reviewed changes

traversaro mentioned this pull request Sep 3, 2019

Updating m_workspace in initSolver() #33

Closed

acauligi mentioned this pull request Sep 18, 2019

Fix call to osqp_setup to reflect current osqp version #34

Closed

traversaro approved these changes Sep 20, 2019

View reviewed changes

S-Dafarra approved these changes Sep 20, 2019

View reviewed changes

traversaro mentioned this pull request Sep 20, 2019

Fix osqp-eigen tag to the latest master robotology/robotology-superbuild#260

Merged

GiulioRomualdi added 2 commits September 23, 2019 15:43

osqp_setup now returns an exit flag instead of OSQPWorkspace*

1404393

osqp now accepts only upper triangular Hessian matrices

2369d49

GiulioRomualdi force-pushed the fix/osqp_0.6.0 branch from 1688022 to 2369d49 Compare September 23, 2019 13:43

- bump version from 0.4.1->0.5.0

ac2a551

- upload a custom version of BasicConfigVersion-SameMinorVersion.cmake.in and WriteBasicConfigVersionFile.cmake to guarantee compatibility with cmake < 3.11 - compatibility ensured only if the required version is 0.5.*

GiulioRomualdi merged commit c469a83 into master Sep 24, 2019

GiulioRomualdi deleted the fix/osqp_0.6.0 branch September 24, 2019 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore compatibility with osqp version 0.6.0 #32

Restore compatibility with osqp version 0.6.0 #32

GiulioRomualdi commented Sep 1, 2019

S-Dafarra Sep 1, 2019

traversaro Sep 1, 2019

GiulioRomualdi Sep 9, 2019

S-Dafarra Sep 9, 2019 •

edited

Loading

GiulioRomualdi Sep 20, 2019

traversaro Sep 20, 2019

GiulioRomualdi Sep 20, 2019

traversaro Sep 20, 2019

S-Dafarra Sep 20, 2019

GiulioRomualdi Sep 20, 2019

traversaro commented Sep 1, 2019

GiulioRomualdi commented Sep 9, 2019

traversaro commented Sep 9, 2019

GiulioRomualdi commented Sep 20, 2019

traversaro commented Sep 20, 2019

traversaro commented Sep 20, 2019

GiulioRomualdi commented Sep 23, 2019 •

edited

Loading

traversaro commented Sep 23, 2019

traversaro commented Sep 23, 2019

GiulioRomualdi commented Sep 23, 2019

GiulioRomualdi commented Sep 23, 2019

Restore compatibility with osqp version 0.6.0 #32

Restore compatibility with osqp version 0.6.0 #32

Conversation

GiulioRomualdi commented Sep 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-Dafarra Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Sep 1, 2019

GiulioRomualdi commented Sep 9, 2019

traversaro commented Sep 9, 2019

GiulioRomualdi commented Sep 20, 2019

traversaro commented Sep 20, 2019

traversaro commented Sep 20, 2019

GiulioRomualdi commented Sep 23, 2019 • edited Loading

traversaro commented Sep 23, 2019

traversaro commented Sep 23, 2019

GiulioRomualdi commented Sep 23, 2019

GiulioRomualdi commented Sep 23, 2019

S-Dafarra Sep 9, 2019 •

edited

Loading

GiulioRomualdi commented Sep 23, 2019 •

edited

Loading