Skip to content

Conversation

@st--
Copy link
Contributor

@st-- st-- commented Jan 23, 2022

The docstring of eigvals referred to eigen! for keyword arguments, but eigen!'s docstring doesn't actually say anything and you actually have to check eigen.

I've also made some other docstrings more consistent ("Compute" vs "Computes" etc).

Question: the docstring of eigen(A, B) -> GeneralizedEigen currently refers to eigen! for keyword arguments - which is similarly unhelpful. The only accepted kwarg of the eigen!(A, B) methods is sortby, similar to eigen(A). Would it not be more userfriendly to simply mention sortby (and refer to eigen(A) docstring for detailed comments)? Would be happy to change it in this PR as well, if you like.

@ViralBShah ViralBShah added the docs This change adds or pertains to documentation label Jan 23, 2022
@dkarrasch dkarrasch added the linear algebra Linear algebra label Jan 24, 2022
@ViralBShah
Copy link
Member

Please do update this PR as per your suggestion to make it user friendly. Thank you!

@st--
Copy link
Contributor Author

st-- commented Jan 26, 2022

@ViralBShah thanks, I've now done that (by copying the corresponding section from the eigen(A) docstring).

@st--
Copy link
Contributor Author

st-- commented Jan 26, 2022

Another question having been looking at the code: while there is eigen(::Diagonal), there is no eigen!(::Diagonal) - this makes sense on one level, as there's no need for making a copy in the first place - but on another level, this would mean code relying on eigen! to avoid allocations would not work for Diagonal matrices.

So should there be an eigen!(A::Diagonal) = eigen(A)?

@st--
Copy link
Contributor Author

st-- commented Jan 26, 2022

More consistency questions:

Some of the implementations such as eigen(::SymTridiagonal) don't actually accept the keyword arguments of the other implementations, scale, permute, sortby. (eigen(::Diagonal) does accept even permute and scale though it just ignores them.)

Should these also wrap the Lapack result in sorteig! before passing it to the Eigen constructor? And have dummy kwargs for scale and permute?

@ViralBShah
Copy link
Member

I'm going to merge this and let's do a separate issue for the other improvements.

@dkarrasch dkarrasch merged commit 70e2432 into JuliaLang:master Feb 7, 2022
@st-- st-- deleted the st/eigen_doc_fix branch February 14, 2022 16:04
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs This change adds or pertains to documentation linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants