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

[Breaking] Use NonlinearSolve for all root finding needs #203

Merged
merged 12 commits into from
Feb 22, 2024

Conversation

avik-pal
Copy link
Member

1 downside is that the API for ManifoldProjection will become a bit more verbose, because we are no longer solving a singular root-finding problem (which NLsolve does with pinv IIRC). Instead for in place problems we have to specify resid_prototype, no such restriction for OOP g (yes those are supported now!!).

If the internal solver fails, we terminate the solution process, unlike before where we just keep going.

Initial Benchmarks

For the first test in manifold_tests.jl

# Old Version
julia> @benchmark sol = solve(prob, Vern7(), callback = cb)
BenchmarkTools.Trial: 1054 samples with 1 evaluation.
 Range (min  max):  4.466 ms   12.455 ms  ┊ GC (min  max): 0.00%  59.69%
 Time  (median):     4.633 ms               ┊ GC (median):    0.00%
 Time  (mean ± σ):   4.744 ms ± 745.682 μs  ┊ GC (mean ± σ):  1.74% ±  6.35%

  ▄█▁                                                          
  ███▇▃▃▂▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▂▂ ▂
  4.47 ms         Histogram: frequency by time        10.6 ms <

 Memory estimate: 1.02 MiB, allocs estimate: 10444.

# This PR
julia> @benchmark sol = solve(prob, Vern7(), callback = cb)
BenchmarkTools.Trial: 6087 samples with 1 evaluation.
 Range (min  max):  558.404 μs    3.089 ms  ┊ GC (min  max):  0.00%  54.91%
 Time  (median):     599.674 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   817.805 μs ± 392.500 μs  ┊ GC (mean ± σ):  24.21% ± 23.11%

  ▇█▇▄▃▁                                 ▁▂▃▃▃▃▃▃▃▂▂▂▂▁▂▁       ▂
  █████████▆▅▅▃▃▅▄▁▅▃▃▃▅▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄▄██████████████████▆▇▆▆▆ █
  558 μs        Histogram: log(frequency) by time       1.75 ms <

 Memory estimate: 11.68 MiB, allocs estimate: 11297.

@avik-pal avik-pal marked this pull request as draft February 20, 2024 16:49
@avik-pal
Copy link
Member Author

Needs SciML/RecursiveArrayTools.jl#356

@avik-pal
Copy link
Member Author

Also ideally needs SciML/NonlinearSolve.jl#382

@avik-pal
Copy link
Member Author

avik-pal commented Feb 20, 2024

TODO

  • Remove Manifest
  • Remove SciMLSensitivity from Dependency list

@avik-pal avik-pal marked this pull request as ready for review February 20, 2024 19:02
@avik-pal avik-pal changed the title Use NonlinearSolve for all root finding needs [Breaking] Use NonlinearSolve for all root finding needs Feb 20, 2024
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@avik-pal avik-pal force-pushed the ap/nonlinearsolve branch 8 times, most recently from 8ef7b1e to 9f705b0 Compare February 21, 2024 19:15
@avik-pal
Copy link
Member Author

Tests pass, I am bumping the project version

@ChrisRackauckas ChrisRackauckas merged commit 89663bb into master Feb 22, 2024
4 of 11 checks passed
@ChrisRackauckas ChrisRackauckas deleted the ap/nonlinearsolve branch February 22, 2024 03:12
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