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

LinearAlgebra: move alg to a keyword argument in symmetric eigen #55481

Closed
wants to merge 4 commits into from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 13, 2024

Currently, eigen for symmetric matrices accepts an alg as the second positional argument, but this departs from the pattern of eigen(A, B) representing a generalized eigenvalue problem. This PR moves alg to a keyword argument instead. This prevents packages from specializing on alg, but I'm unsure if there's demand for that.

Incidentally, alg as a keyword argument in eigen is also consistent with how it is passed to the sort family of functions, although there's no direct relation between these.

@jishnub jishnub added the linear algebra Linear algebra label Aug 13, 2024
@LilithHafner
Copy link
Member

Losing the ability to specialize on a custom alg type seems like a pretty big loss. How would one write EigenAlgorithms.jl? FWIW, specialization on algorithms in sorting using the public API is a mess.

@ViralBShah
Copy link
Member

ViralBShah commented Aug 13, 2024

Aren't eigenvalue computations large and slow enough that there wouldn't be gains from specialization? Is the argument that it would make the rest of the computation type unstable in some way?

@jishnub
Copy link
Contributor Author

jishnub commented Aug 13, 2024

Perhaps we may have an internal function that specialises on alg? It doesn't need to be eigen. This function may be public, and methods may be added by other packages.

@LilithHafner
Copy link
Member

@ViralBShah, the argument is that the extension API is not as discoverable. Performance and type stability should be fine.

# LinearAlgebra.jl
public eigen
eigen(::AbstractMatrix, ::Algorithm) = ...

---

# EigenAlgorithms.jl
eigen(::AbstractMatrix, ::MyFancyAlgorithm) = ...

vs

# LinearAlgebra.jl
public eigen, eigen_dispatch
eigen(x::AbstractMatrix; alg::Algorithm) = eigen_dispatch(x, a)
eigen_dispatch(::AbstracMatrix, ::Algorithm) = ...

---

# EigenAlgorithms.jl
eigen_dispatch(::AbstractMatrix, ::MyFancyAlgorithm) = ...

@jishnub
Copy link
Contributor Author

jishnub commented Aug 14, 2024

I don't think this needs to be particularly discoverable, as this isn't user-facing. We may add a note to developers in the docstring of eigen to point to this function.

@DilumAluthge
Copy link
Member

We have moved the LinearAlgebra stdlib to an external repo: https://github.com/JuliaLang/LinearAlgebra.jl

@jishnub If you think that this PR is still relevant, please open a new PR on the LinearAlgebra.jl repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants