KrylovKit.jl icon indicating copy to clipboard operation
KrylovKit.jl copied to clipboard

VectorInterface

Open lkdvos opened this issue 3 years ago • 5 comments

rewrite algorithms in terms of VectorInterface methods.

  • Some additional cleanup can be done in terms of scalartype
  • algorithms can be written in terms of bangbang-methods, possibly allowing tuples as vectors?

lkdvos avatar Dec 01 '22 22:12 lkdvos

Codecov Report

Attention: Patch coverage is 81.79272% with 65 lines in your changes are missing coverage. Please review.

Project coverage is 82.05%. Comparing base (e4932b4) to head (7a27157).

Files Patch % Lines
src/innerproductvec.jl 0.00% 18 Missing :warning:
src/recursivevec.jl 51.85% 13 Missing :warning:
src/adrules/linsolve.jl 33.33% 10 Missing :warning:
src/eigsolve/golubye.jl 87.03% 7 Missing :warning:
src/factorizations/lanczos.jl 82.35% 6 Missing :warning:
src/factorizations/gkl.jl 87.50% 4 Missing :warning:
src/orthonormal.jl 95.23% 3 Missing :warning:
src/linsolve/bicgstab.jl 94.87% 2 Missing :warning:
src/matrixfun/expintegrator.jl 87.50% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   82.30%   82.05%   -0.25%     
==========================================
  Files          27       27              
  Lines        2741     2753      +12     
==========================================
+ Hits         2256     2259       +3     
- Misses        485      494       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 01 '22 22:12 codecov[bot]

Hi Lukas; thanks for this great work. I was indeed considering switching to bangbang methods when adopting VectorInterface, so if you have time to try this out and see if there would be any unexpected consequences or complications, feel free to do so ...

Jutho avatar Dec 01 '22 23:12 Jutho

@Jutho I seem to be stuck with the geneigsolve for out-of-place vectors, although I can't seem to locate what's going wrong. It produces matrices that are not positive definite, such that geneigh! fails, but I have absolutely no clue why this would be different for the out-of-place methods. I might look into it later this week, but any help is definitely welcome. (tests are ran by just commenting out modules "svdsolve.jl" and "expintegrator.jl")

lkdvos avatar Dec 07 '22 15:12 lkdvos

Some small remaining remarks:

  • I would like to still restructure some of the tests, such that there is less duplicated code in the tests, and less tests run multiple times.
  • Somehow this still had the eigenvalue ad rules that are not in the main branch. It might be better to add these in a separate PR, which immediately adds the ADrules as a package extension.
  • There are no tests for the eigsolve ad, but this should probably also happen in the separate pr as mentioned above

lkdvos avatar Feb 23 '24 14:02 lkdvos

So, if the lights turn green, this should be ready for review.

I excluded AD rules from the new functionality, because we should probably rewrite that section anyways, using Package extensions.

Otherwise, I think everything should now work with VectorInterface, and the tests have been reorganised to extensively test everything with regular vectors, and then do some small additional tests with MinimalVec-type vectors.

In principle I think we can also remove/deprecate RecursiveVec and InnerProductVec, but this would probably require some deprecation update first?

lkdvos avatar Mar 05 '24 15:03 lkdvos

Nightly failures seem due to Zygote failing?

Jutho avatar Mar 08 '24 16:03 Jutho

I think so, it also seems to vary quite a bit depending on what version of nightly, so I think we can ignore this. We'll refactor that part anyways when we move the AD support to a package extension I guess.

lkdvos avatar Mar 08 '24 17:03 lkdvos