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

Use LinearAlgebra.Diagonal instead of defining SDiagonal type #533

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

simonbyrne
Copy link
Contributor

Fixes #531. I've kept SDiagonal as a type alias for compatibility.

Fixes JuliaArrays#531. I've kept SDiagonal as a type alias for compatibility.
src/SDiagonal.jl Outdated
*(A::StaticMatrix, D::SDiagonal) = scalem(A,D.diag)
*(D::SDiagonal, A::StaticMatrix) = scalem(D.diag,A)
# define specific methods to avoid allocating mutable arrays
*(A::StaticMatrix, D::SDiagonal) = A .* D.diag'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to time this against scalem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it does seem to be about 10x slower for a 4x4 matrix. I wonder why it's so much slower?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I forgot to use $ interpolation. They're more or less the same.

@mschauer
Copy link
Collaborator

Thank you. I find it surprising that nothing is lost if a static array object is not inheriting into StaticArray or is there some trade-off I am missing?

@simonbyrne
Copy link
Contributor Author

I'm not sure I understand what you mean. This is just using the broadcasting of static arrays

@c42f
Copy link
Member

c42f commented Oct 28, 2018

I think the point is that previously foo(m::StaticMatrix) would match SDiagonal, whereas it won't anymore. Now we get foo(m::Diagonal) to match instead.

I'm not sure which is best to be quite honest. One clear advantage of this PR is that we get to delete a bunch of code.

@simonbyrne
Copy link
Contributor Author

Ah, I see. I personally think it makes sense to use Diagonal since
(a) that was the whole point of making Diagonal parametric
(b) we already do the same for factorization objects like Cholesky
(c) it requires less code to maintain

If you need a way to determine if the underlying structure is a StaticArray, perhaps that would be better done using traits?

There is probably still room to improve Diagonal in LinearAlgebra so that fewer method overrides are needed here.

@c42f
Copy link
Member

c42f commented Oct 28, 2018

Yes, I agree we can't have both, and that traits are a way out. I do think the changes here are probably a good idea, I'm just not using SDiagonal myself.

@c42f
Copy link
Member

c42f commented Oct 28, 2018

@mschauer it looks like you contributed SDiagonal originally. Would this change affect your packages much?

@andyferris
Copy link
Member

Fundamentally, users should be able to wrap StaticArrays in their own array types without problems or caveats. I totally support that.

To my mind, we should see if we can support the "trait" of static-sizedness directly through length, size, axes and keys. We first need SUnitRange to work properly as it's own keys.

@c42f
Copy link
Member

c42f commented Oct 29, 2018

@andyferris you're right. I think we postponed this in the past (v0.5??) because the compiler couldn't deal with it. But the compiler has improved hugely since then.

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.

4 participants