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

Porting over some low cost Algorithms #257

Merged
merged 10 commits into from
Oct 25, 2023

Conversation

avik-pal
Copy link
Member

@avik-pal avik-pal commented Oct 20, 2023

  • Broyden
    • Inplace
    • OOP
    • Integrate into the Test Suite
      • Basic Tests
      • 23 Test Problems
  • Klement
    • Inplace
    • OOP
    • Integrate into the Test Suite
      • Basic Tests
      • 23 Test Problems

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #257 (bd5af95) into master (0dac21a) will decrease coverage by 1.66%.
The diff coverage is 91.04%.

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
- Coverage   93.90%   92.25%   -1.66%     
==========================================
  Files          15       18       +3     
  Lines        1231     1614     +383     
==========================================
+ Hits         1156     1489     +333     
- Misses         75      125      +50     
Files Coverage Δ
src/NonlinearSolve.jl 90.00% <100.00%> (+0.52%) ⬆️
src/default.jl 77.21% <ø> (-1.27%) ⬇️
src/dfsane.jl 97.90% <ø> (-2.10%) ⬇️
src/levenberg.jl 98.46% <100.00%> (ø)
src/pseudotransient.jl 100.00% <100.00%> (ø)
src/raphson.jl 100.00% <100.00%> (ø)
src/trustRegion.jl 95.85% <100.00%> (-3.51%) ⬇️
src/lbroyden.jl 98.37% <98.37%> (ø)
src/broyden.jl 96.05% <96.05%> (ø)
src/utils.jl 84.32% <89.65%> (+1.15%) ⬆️
... and 2 more

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

@avik-pal
Copy link
Member Author

For Klement if we allow any general linear solve, how do we go about doing the singularity check for resetting? For LU, we could do the check on the diagonal of U, but that won't work for other solvers. Should we use cond(J) > inv(tol), or is there a better way to do this?

cc @ChrisRackauckas

@avik-pal
Copy link
Member Author

I have added both Klement and Broyden. The naming is a bit unfortunate since SimpleNonlinearSolve reserves the names instead of Simple****.

Broyden is faster than the one in SimpleNonlinearSolve.

Klement as expected seems to spend a significant amount of time in singularity checking. Someone with more familiarity with LinearAlgebra needs to chime in on how to handle this in the general case. I can add a branching based on LU factorization and default to that.

@avik-pal
Copy link
Member Author

I am reusing the factorization for singularity if the user specifies LUFactorization, else we fallback to ArrayInterface.issingular

julia> @benchmark solve(probN, GeneralKlement(; linsolve=LUFactorization()))
BenchmarkTools.Trial: 23 samples with 1 evaluation.
 Range (min  max):  168.920 ms  283.473 ms  ┊ GC (min  max): 0.00%  40.56%
 Time  (median):     218.867 ms               ┊ GC (median):    1.57%
 Time  (mean ± σ):   219.950 ms ±  37.673 ms  ┊ GC (mean ± σ):  8.37% ± 11.92%

  ███           ▁ █   ▁   ▁ ▁     ▁   █ ▁ ▁      ▁█ ▁     ▁   ▁  
  ███▁▁▁▁▁▁▁▁▁▁▁█▁█▁▁▁█▁▁▁█▁█▁▁▁▁▁█▁▁▁█▁█▁█▁▁▁▁▁▁██▁█▁▁▁▁▁█▁▁▁█ ▁
  169 ms           Histogram: frequency by time          283 ms <

 Memory estimate: 252.29 MiB, allocs estimate: 201.

julia> @benchmark solve(probN, GeneralKlement(; linsolve=QRFactorization()))
BenchmarkTools.Trial: 12 samples with 1 evaluation.
 Range (min  max):  382.058 ms  475.142 ms  ┊ GC (min  max): 1.69%  20.59%
 Time  (median):     440.708 ms               ┊ GC (median):    1.52%
 Time  (mean ± σ):   433.578 ms ±  36.193 ms  ┊ GC (mean ± σ):  5.63% ±  7.69%

  █ ▁           ▁                    ▁ ▁ ▁       ▁ ▁        ▁ █  
  █▁█▁▁▁▁▁▁▁▁▁▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█▁█▁█▁▁▁▁▁▁▁█▁█▁▁▁▁▁▁▁▁█▁█ ▁
  382 ms           Histogram: frequency by time          475 ms <

 Memory estimate: 309.65 MiB, allocs estimate: 229.

julia> @benchmark solve(probN, GeneralKlement(; linsolve=nothing))
BenchmarkTools.Trial: 18 samples with 1 evaluation.
 Range (min  max):  232.665 ms  382.726 ms  ┊ GC (min  max):  0.87%  36.40%
 Time  (median):     274.173 ms               ┊ GC (median):     2.18%
 Time  (mean ± σ):   283.057 ms ±  47.992 ms  ┊ GC (mean ± σ):  10.11% ± 12.59%

  █▁   ▁                                                         
  ██▁▁▁█▁▁▁▁▁▁▆▁▁▁▁▁▁▁▁▆▁▁▁▁▁▆▁▁▁▁▁▁▆▆▁▆▆▆▁▁▁▆▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▆ ▁
  233 ms           Histogram: frequency by time          383 ms <

 Memory estimate: 305.70 MiB, allocs estimate: 192.

on a problem with 1000 states.

@avik-pal avik-pal force-pushed the ap/more_alg branch 7 times, most recently from d2251ba to 4ce655d Compare October 22, 2023 19:42
@avik-pal avik-pal changed the title Porting over some low cost Algorithms [WIP] Porting over some low cost Algorithms Oct 23, 2023
@avik-pal
Copy link
Member Author

Marking it as WIP, I want to add LBroyden to this as well!

@avik-pal avik-pal changed the title [WIP] Porting over some low cost Algorithms Porting over some low cost Algorithms Oct 23, 2023
@avik-pal avik-pal force-pushed the ap/more_alg branch 2 times, most recently from 910a30a to 4dc40bc Compare October 23, 2023 14:15
@avik-pal
Copy link
Member Author

@ChrisRackauckas this is ready to go!

@avik-pal
Copy link
Member Author

@ChrisRackauckas let's merge this one?

@ChrisRackauckas ChrisRackauckas merged commit b300050 into SciML:master Oct 25, 2023
6 of 9 checks passed
@avik-pal avik-pal deleted the ap/more_alg branch October 25, 2023 14:37
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