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

Why is the SparseMatrixCOO mutable? #44

Closed
albertomercurio opened this issue Jun 2, 2024 · 7 comments
Closed

Why is the SparseMatrixCOO mutable? #44

albertomercurio opened this issue Jun 2, 2024 · 7 comments

Comments

@albertomercurio
Copy link

Hello,

looking at the code I noticed that the SparseMatrixCOO constructor is mutable, while the SparseMatrixCSC one is not.

I think that it can be safely converted into immutable. Am I wrong?

@dpo
Copy link
Member

dpo commented Jun 2, 2024

It’s useful to keep it mutable because in optimization, a Hessian or Jacobian often retains the same sparsity pattern; only the nonzero values change from one iteration to the next. That is what happens in NLPModels.jl.

@albertomercurio
Copy link
Author

But than one can still change the vals vector inplace. The struct would be immutable, but the vectors inside not. Even for the SparseMatrixCSC I can update its nonzero values.

@dpo
Copy link
Member

dpo commented Jun 2, 2024

Well, yes, I suppose you're right. Do you need the struct to be immutable for a special reason? Or just for uniformity with SparseMatrixCSC?

@albertomercurio
Copy link
Author

Yes, for sure because of uniformity. But also because immutable structs would be also faster AFAIK.

@dpo
Copy link
Member

dpo commented Jun 3, 2024

Sounds good. Would you like to submit a PR?

@albertomercurio
Copy link
Author

Yes, why not. When I will have some free time I will do it.

@dpo
Copy link
Member

dpo commented Dec 21, 2024

Done in #54.

@dpo dpo closed this as completed Dec 21, 2024
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

No branches or pull requests

2 participants