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

Use ITP as root-finding algorithm in planar layer #343

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Use ITP as root-finding algorithm in planar layer #343

merged 4 commits into from
Nov 6, 2024

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Nov 4, 2024

As recommended in EnzymeAD/Enzyme.jl#2035 (comment) for increased robustness.

ITP was added in Roots 1.3.15: JuliaMath/Roots.jl#274

Edit: A quick comparison:

julia> using Bijectors

julia> x = randn(); y = expm1(randn()); z = randn();

julia> Bijectors.find_alpha(x, y, z)
0.20145381857054057 # PR
0.20145381857054054 # master

On the master branch:

julia> using Bijectors, Chairmarks

julia> @be (randn(), expm1(randn()), randn()) Bijectors.find_alpha(_[1], _[2], _[3])
Benchmark: 3014 samples with 15 evaluations
 min    1.350 μs
 median 2.031 μs
 mean   2.052 μs
 max    3.578 μs

julia> @be (randn(), expm1(randn()), randn()) Bijectors.find_alpha(_[1], _[2], _[3])
Benchmark: 3440 samples with 13 evaluations
 min    1.327 μs
 median 2.053 μs
 mean   2.078 μs
 max    9.151 μs

On this PR:

julia> using Bijectors, Chairmarks

julia> @be (randn(), expm1(randn()), randn()) Bijectors.find_alpha(_[1], _[2], _[3])
Benchmark: 2969 samples with 34 evaluations
 min    686.294 ns
 median 890.941 ns
 mean   909.760 ns
 max    1.614 μs

julia> @be (randn(), expm1(randn()), randn()) Bijectors.find_alpha(_[1], _[2], _[3])
Benchmark: 3790 samples with 26 evaluations
 min    663.462 ns
 median 910.250 ns
 mean   929.133 ns
 max    3.385 μs

devmotion and others added 4 commits November 4, 2024 17:20
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Thanks @devmotion -- this looks good. The failing tests are due to Enzyme's lack of support for Julia 1.11.

@penelopeysm can you help separate Enzyme tests into a separate CI task and allow it to fail for PRs?

@yebai yebai merged commit 5c1feeb into master Nov 6, 2024
23 of 27 checks passed
@yebai yebai deleted the dw/itp branch November 6, 2024 11:21
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