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

Return Symmetric Hessians by default? #154

Closed
adrhill opened this issue Aug 8, 2024 · 4 comments
Closed

Return Symmetric Hessians by default? #154

adrhill opened this issue Aug 8, 2024 · 4 comments
Labels
discussion Discuss design decisions / future direction of the package

Comments

@adrhill
Copy link
Owner

adrhill commented Aug 8, 2024

In #151, the question popped up whether hessian_sparsity should return a Symmetric{Bool, SparseMatrixCSC{Bool, Int64}} instead of the current SparseMatrixCSC{Bool, Int64}.

Since this can be considered a breaking change, this feature would have to wait until we have more reasons to tag a breaking v0.7 release.

@gdalle @Vaibhavdixit02 @amontoison I would be interested in your opinions on this!

Related issue: #14

@adrhill adrhill added the discussion Discuss design decisions / future direction of the package label Aug 8, 2024
@gdalle
Copy link
Collaborator

gdalle commented Aug 8, 2024

For SparseMatrixColorings, the type of the sparsity pattern S doesn't really matter at first glance: I convert everything to SparseMatrixCSC before creating my graphs:

https://github.com/gdalle/SparseMatrixColorings.jl/blob/a0f516c7b31ca8db9c49bf51c758caa00e73fa06/src/graph.jl#L13-L19

However, the sparsity pattern is also an important information for the type of the future decompressed matrix A.
With SparseMatrixColorings v0.4, the type of S is used to prepare decompression, anticipating that the type of A will be identical. Ideally we would also know the type of A in advance, but we don't always, so S is our best approximation.

In my view, we're rather unlikely to decompress in-place into a Symmetric because it is not mutable outside the diagonal. Therefore, I advocate for keeping SparseMatrixCSC by default.

Related:

@Vaibhavdixit02
Copy link

Solvers (with recency bias of ipopt) typically only require the lower or upper triangular, and for me the breakage wouldn't be a big problem yet. Based on the results in the linked PR this gives a median ~15% improvement in the performance which seems pretty useful to me!

@amontoison
Copy link

The issue is in the coloring, we need both triangles.
But I agree that using only triangle is relevant in terms of storage and we only need one triangle for the decompression.

@adrhill
Copy link
Owner Author

adrhill commented Aug 9, 2024

Alright, let's keep the default SparseMatrixCSC{Bool, Int64} and add an option for Symmetric outputs via #14.
Thanks everyone!

@adrhill adrhill closed this as completed Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discuss design decisions / future direction of the package
Projects
None yet
Development

No branches or pull requests

4 participants