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

Correctly iterate over row major sparse matrices #54

Closed
wants to merge 1 commit into from

Conversation

mpowelson
Copy link
Contributor

Changes createOsqpSparseMatrix to iterate to eigenSparseMatrix.outerSize() instead of cols().

@S-Dafarra
Copy link
Collaborator

Hi @mpowelson. Thanks a lot for the PR! Out of sheer curiosity, how did you manage to find this? Do you have some piece of code that is failing without this change?

@mpowelson
Copy link
Contributor Author

We are using osqp-eigen in trajopt_ros and our matrices are row major. I don't remember the symptoms, but something was going wrong and when I debugged it, I saw the matrix was the wrong size.

@S-Dafarra
Copy link
Collaborator

Thanks for the explanation! I tried to investigate a little more the issue. Actually, when developing we mainly focused on "Compressed Sparse-Column" (CSC) as this is what osqp is expecting (https://github.com/oxfordcontrol/osqp/blob/master/include/types.h#L21-L29l). I guess that by passing a "Compressed Sparse-Row" (CSR) the net effect is that the matrix may result transposed.

On the Eigen side, it is possible to pass from CSR to CSC with a simple assignment. Probably it is safer to save the input matrix into a CSC to avoid this kind of problem. I tried to implement this in #57. @mpowelson can you try to check that out to see if it solves your problem?

@mpowelson
Copy link
Contributor Author

Yes, it seems like that solves it. I'll close this.

@mpowelson mpowelson closed this May 14, 2020
@mpowelson mpowelson deleted the bugfix branch May 26, 2020 23:25
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