SciML / LinearSolve.jl

LinearSolve.jl: High-Performance Unified Interface for Linear Solvers in Julia. Easily switch between factorization and Krylov methods, add preconditioners, and all in one interface.
https://docs.sciml.ai/LinearSolve/stable/
Other
245 stars 52 forks source link

Mutable cache form #303

Closed ChrisRackauckas closed 1 year ago

ChrisRackauckas commented 1 year ago

I think this is required to fix https://github.com/SciML/LinearSolve.jl/issues/302 . Otherwise there is no way to set some kind of "has been factorized". The reason for the functional immutable form was to be easier on the compiler, but I don't think that has worked out in the end. It has been a bit unergonomic to have to keep finding the latest cache, and in scenarios where it's kept longer (i.e. init the ODE solver) it will allocate just the same, while when it's in a more constrained context (solve in the ODE solver) the compiler will elide the allocation for both the mutable and immutable forms these days. Also, this puts LinearSolve more in line with the rest of the SciML interface libraries.

ChrisRackauckas commented 1 year ago

This is majorly breaking so it will be 2.0.

fredrikekre commented 1 year ago

This is majorly breaking so it will be 2.0.

Can I request to change the default behavior of copying the matrix/vector if there will be a breaking release? :)

ChrisRackauckas commented 1 year ago

What's the requested change?

fredrikekre commented 1 year ago

To not copy by default, flipping these https://github.com/SciML/LinearSolve.jl/blob/252aaa2ab24a6924c2ce78c981cef56a9a937210/src/common.jl#L146-L147 to true.

j-fu commented 1 year ago

Hi, this seems to be an interesting improvement regarding usability of the API. I will try to test this with VoronoiFVM.jl, I should be able to find some time this week, would report back.

ChrisRackauckas commented 1 year ago

To not copy by default, flipping these

Is there a way to do that and still solve the problem that two solves by default give the same answer? I'm not sure there is?

fredrikekre commented 1 year ago

Do you have an MWE for what you mean?

ChrisRackauckas commented 1 year ago

https://github.com/SciML/LinearSolve.jl/issues/302 is something that for example I'm not sure would work.

j-fu commented 1 year ago

Seems to work for me, simplifies things big time! I one place I had a mutable struct which just wrapped a LinearCache to carry around...

But why make this immediately breaking ? IMHO it could be possible to keep the old API via solve and the new one via solve! . May be it could be deprecated and removed later.

This also would prevent making packages using LinearSolve 1.0 and LinearSolve 2.0 incompatible.

ChrisRackauckas commented 1 year ago

I should/will go back and add deprecation paths, but it's much easier to test things by removing the syntax that shouldn't be used and then after everything works adding the deprecation paths back.

codecov[bot] commented 1 year ago

Codecov Report

Merging #303 (ac35c9b) into main (252aaa2) will increase coverage by 53.82%. The diff coverage is 40.00%.

@@            Coverage Diff             @@
##            main     #303       +/-   ##
==========================================
+ Coverage   0.00%   53.82%   +53.82%     
==========================================
  Files         11       14        +3     
  Lines        862     1007      +145     
==========================================
+ Hits           0      542      +542     
+ Misses       862      465      -397     
Impacted Files Coverage Δ
ext/LinearSolveHYPRE.jl 0.00% <0.00%> (ø)
lib/LinearSolveCUDA/src/LinearSolveCUDA.jl 0.00% <0.00%> (ø)
lib/LinearSolvePardiso/src/LinearSolvePardiso.jl 0.00% <0.00%> (ø)
src/LinearSolve.jl 82.75% <ø> (+82.75%) :arrow_up:
src/default.jl 29.41% <0.00%> (+29.41%) :arrow_up:
src/deprecated.jl 0.00% <0.00%> (ø)
src/simplelu.jl 71.59% <71.42%> (+71.59%) :arrow_up:
src/solve_function.jl 90.90% <75.00%> (+90.90%) :arrow_up:
src/factorization.jl 64.90% <78.04%> (+64.90%) :arrow_up:
src/iterative_wrappers.jl 78.28% <85.71%> (+78.28%) :arrow_up:
... and 1 more

... and 3 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ChrisRackauckas commented 1 year ago

@fredrikekre if you think that's solvable, please open an issue and let's discuss what to do there. I don't plan to tag until the end of the week so this can get some more testing, and a few more necessary changes in.

j-fu commented 1 year ago

Checked for compatibility of the immutatable cache stuff - this works as well :+1: