search

JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.73k stars 5.49k forks source link

problems with new Eigen API #41523

Open RalphAS opened 3 years ago

RalphAS commented 3 years ago

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.
RalphAS commented 3 years ago

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

KlausC commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/JuliaLang/julia/pull/41578.