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

problems with new Eigen API #41523

Open
RalphAS opened this issue Jul 9, 2021 · 4 comments
Open

problems with new Eigen API #41523

RalphAS opened this issue Jul 9, 2021 · 4 comments
Labels
domain:docs This change adds or pertains to documentation domain:linear algebra Linear algebra kind:bug Indicates an unexpected problem or unintended behavior

Comments

@RalphAS
Copy link

RalphAS commented Jul 9, 2021

There are several.

  1. Incompleteness. One needs the norm of the balanced matrix to properly interpret the condition numbers. Cf. LAPACK User Guide. Currently it is discarded.
  2. Inconsistency. For real matrices, but not complex ones, the reciprocal condition numbers are inverted. If you call the fields "rcondX", it is misleading to do this.
  3. Poor choice of keyword names. "jvl", "jce", "jcv" are noninformative and inconsistent with typical Julia patterns.
  4. Documentation.
  5. Tests.
@andreasnoack andreasnoack added kind:bug Indicates an unexpected problem or unintended behavior domain:linear algebra Linear algebra domain:docs This change adds or pertains to documentation labels Jul 9, 2021
@RalphAS
Copy link
Author

RalphAS commented Jul 13, 2021

I forgot to snarl at the responsible parties: @KlausC @stevengj
Ref #38483

@KlausC
Copy link
Contributor

KlausC commented Jul 13, 2021

@RalphAS: thanks for your constructive critics. I agree, that this PR was kind of not complete, when I posted it.
I would like to make the following changes, corresponding to your issues:

  1. Variable abnrm, which is calculated by geevx could be stored as an extra field of Eigen to make it available for estimations.
  2. Inverting the rconde and rcondv vectors is not a good idea. That should not be done in neither case. This is a bug.
  3. The keyword argument names are inspired by the LAPACK name jobvl, meaning "calculate left vectors". And they should be short! Though jvl does not correspond exactly to jobvl but means "return left eigenvectors". jvr means "return right eigenvectors", jce means "return condition numbers for eigenvalues" and jcv means "return condition numbers for eigenvectors". Feel free to propose better names for those boolean arguments.
  4. Documentation and tests for the new features introduced by this PR are completely missing. Mea culpa.
  5. dito

I will have time to work out a PR at the earliest in a few weeks, hopefully, when I will be back from hospital. You are predestined for reviewing that.
Alternatively you could make the PR, which I could review.

@RalphAS
Copy link
Author

RalphAS commented Jul 14, 2021

For keywords perhaps "left", "right", "econdition", "vcondition" would be good. (I summon @dpsanders for advice.)

Best wishes for successful procedures and gentle & speedy recovery.

@andreasnoack
Copy link
Member

We have very little time before the 1.7 release and we since the fixes most likely will involve API changes, I think the best way to proceed here is to revert the original PR and get a new version of this merged in the 1.8 cycle. Hence, I've opened #41578.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation domain:linear algebra Linear algebra kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants