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

VectorInterface #65

Merged
merged 61 commits into from
Mar 14, 2024
Merged

VectorInterface #65

merged 61 commits into from
Mar 14, 2024

Conversation

lkdvos
Copy link
Collaborator

@lkdvos lkdvos commented Dec 1, 2022

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?

@codecov
Copy link

codecov bot commented Dec 1, 2022

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 ⚠️
src/recursivevec.jl 51.85% 13 Missing ⚠️
src/adrules/linsolve.jl 33.33% 10 Missing ⚠️
src/eigsolve/golubye.jl 87.03% 7 Missing ⚠️
src/factorizations/lanczos.jl 82.35% 6 Missing ⚠️
src/factorizations/gkl.jl 87.50% 4 Missing ⚠️
src/orthonormal.jl 95.23% 3 Missing ⚠️
src/linsolve/bicgstab.jl 94.87% 2 Missing ⚠️
src/matrixfun/expintegrator.jl 87.50% 2 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jutho
Copy link
Owner

Jutho commented Dec 1, 2022

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 ...

src/adrules/linsolve.jl Outdated Show resolved Hide resolved
@lkdvos
Copy link
Collaborator Author

lkdvos commented Feb 23, 2024

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
Copy link
Collaborator Author

lkdvos commented Mar 5, 2024

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?

Copy link
Owner

@Jutho Jutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing a first bunch of review comments on everything in scr. I will now continue reviewing everything in test.

src/adrules/linsolve.jl Outdated Show resolved Hide resolved
src/dense/givens.jl Outdated Show resolved Hide resolved
src/eigsolve/arnoldi.jl Outdated Show resolved Hide resolved
src/eigsolve/golubye.jl Outdated Show resolved Hide resolved
src/eigsolve/lanczos.jl Outdated Show resolved Hide resolved
src/factorizations/lanczos.jl Outdated Show resolved Hide resolved
src/linsolve/bicgstab.jl Outdated Show resolved Hide resolved
src/linsolve/cg.jl Outdated Show resolved Hide resolved
src/linsolve/gmres.jl Outdated Show resolved Hide resolved
src/recursivevec.jl Outdated Show resolved Hide resolved
Copy link
Owner

@Jutho Jutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the tests, I have mixed feelings. The reduced runtime is very good, but it seems this comes with quite a bit of near-trivial code duplication.

test/runtests.jl Outdated Show resolved Hide resolved
test/factorize.jl Outdated Show resolved Hide resolved
test/gklfactorize.jl Outdated Show resolved Hide resolved
@Jutho
Copy link
Owner

Jutho commented Mar 8, 2024

Nightly failures seem due to Zygote failing?

@lkdvos
Copy link
Collaborator Author

lkdvos commented Mar 8, 2024

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.

@Jutho Jutho merged commit a9fdb62 into Jutho:master Mar 14, 2024
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants