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

Replace NonlinearSolve with Roots and fix the ReverseDiff adjoint of find_alpha #202

Merged
merged 2 commits into from
Oct 16, 2021

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Oct 15, 2021

This PR replaces NonlinearSolve with Roots for solving the root finding problem in the inverse of the planar layer.

I fixed some type inference problems in Roots some weeks ago (JuliaMath/Roots.jl#245) and some time ago also the convergence and AD issues of this algorithm were fixed by #126 by using an improved initial bracket and custom adjoints (BTW the ReverseDiff adjoint was never used and broken, it's fixed in this PR as well); so the motivation and reasons for #155 (move from Roots to NonlinearSolve) are gone.

The main motivation for this PR is (apart from the ReverseDiff bugfix) that it reduces the dependencies of Bijectors quite dramatically and hence makes the package more lightweight (#199). With the master branch, removing NonlinearSolve and adding Roots removes and adds the following indirect dependencies (note that all indirect dependencies added by Roots are already present due to NonlinearSolve):

(Bijectors) pkg> rm NonlinearSolve
    Updating `~/.julia/dev/Bijectors/Project.toml`
  [8913a72c] - NonlinearSolve v0.3.11
    Updating `~/.julia/dev/Bijectors/Manifest.toml`
  [79e6a3ab] - Adapt v3.3.1
  [4fba245c] - ArrayInterface v3.1.33
  [62783981] - BitTwiddlingConvenienceFunctions v0.1.0
  [2a0fbf3d] - CPUSummary v0.1.5
  [fb6a15b2] - CloseOpenIntervals v0.1.2
  [38540f10] - CommonSolve v0.2.0
  [bbf7d656] - CommonSubexpressions v0.3.0
  [187b0558] - ConstructionBase v1.3.0
  [e2d170a0] - DataValueInterfaces v1.0.0
  [163ba53b] - DiffResults v1.0.3
  [b552c78f] - DiffRules v1.3.1
  [6a86dc24] - FiniteDiff v2.8.1
  [f6369f11] - ForwardDiff v0.10.21
  [3e5b6fbb] - HostCPUFeatures v0.1.4
  [0e44f5e4] - Hwloc v2.0.0
  [615f187c] - IfElse v0.1.0
  [42fd0dbc] - IterativeSolvers v0.9.1
  [82899510] - IteratorInterfaceExtensions v1.0.0
  [10f19ff3] - LayoutPointers v0.1.3
  [bdcacae8] - LoopVectorization v0.12.85
  [1914dd2f] - MacroTools v0.5.8
  [d125e4d3] - ManualMemory v0.1.6
  [77ba4419] - NaNMath v0.3.5
  [8913a72c] - NonlinearSolve v0.3.11
  [6fe1bfb0] - OffsetArrays v1.10.7
  [f517fe37] - Polyester v0.5.3
  [1d0040c9] - PolyesterWeave v0.1.0
  [3cdcf5f2] - RecipesBase v1.1.2
  [731186ca] - RecursiveArrayTools v2.19.1
  [f2c3362d] - RecursiveFactorization v0.2.4
  [3cdde19b] - SIMDDualNumbers v0.1.0
  [94e857df] - SIMDTypes v0.1.0
  [476501e8] - SLEEFPirates v0.6.27
  [0bca4576] - SciMLBase v1.19.2
  [efcf1570] - Setfield v0.8.0
  [aedffcd0] - Static v0.3.3
  [90137ffa] - StaticArrays v1.2.13
  [7792a7ef] - StrideArraysCore v0.2.5
  [3783bdb8] - TableTraits v1.0.1
  [bd369af6] - Tables v1.6.0
  [8290d209] - ThreadingUtilities v0.4.6
  [a2a6695c] - TreeViews v0.3.0
  [d5829a12] - TriangularSolve v0.1.6
  [3a884ed6] - UnPack v1.0.2
  [3d5dd08c] - VectorizationBase v0.21.13
  [700de1a5] - ZygoteRules v0.2.2
  [e33a78d0] - Hwloc_jll v2.5.0+0
  [9fa8497b] - Future

(Bijectors) pkg> add Roots
   Resolving package versions...
    Updating `~/.julia/dev/Bijectors/Project.toml`
  [f2b01f46] + Roots v1.3.5
    Updating `~/.julia/dev/Bijectors/Manifest.toml`
  [38540f10] + CommonSolve v0.2.0
  [187b0558] + ConstructionBase v1.3.0
  [1914dd2f] + MacroTools v0.5.8
  [f2b01f46] + Roots v1.3.5
  [efcf1570] + Setfield v0.8.0
  [9fa8497b] + Future

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

You tha best David ❤️

@torfjelde torfjelde merged commit 0cc45d3 into master Oct 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the dw/roots branch October 16, 2021 06:14
@torfjelde
Copy link
Member

Eeehh I should have let you merge in case you weren't done with it (realized you hadn't requested a review yet), though it looks good to me. I'll wait with release until you given me a thumbs-up that it's all good 👍

@devmotion
Copy link
Member Author

No worries, all good 👍

@devmotion
Copy link
Member Author

JuliaRegistries/General#46848

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