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

Add parameter to disable early exit of expression evaluation #91

Merged
merged 44 commits into from
Jul 28, 2024

Conversation

nmheim
Copy link
Contributor

@nmheim nmheim commented Jul 1, 2024

Addresses #87.

@MilesCranmer is this roughly what you had in mind? If not, just let me know. If yes, I can also add another test with e.g. a more complicated expression. Let me know what you think!

Copy link
Contributor

github-actions bot commented Jul 1, 2024

Benchmark Results

master 17a4a24... master/17a4a24ef4e93f...
eval/ComplexF32/evaluation 7.33 ± 0.46 ms 7.27 ± 0.46 ms 1.01
eval/ComplexF64/evaluation 9.56 ± 0.67 ms 9.58 ± 0.65 ms 0.998
eval/Float32/derivative 11 ± 1.7 ms 11 ± 1.8 ms 1.01
eval/Float32/derivative_turbo 11 ± 1.8 ms 10.9 ± 1.7 ms 1.02
eval/Float32/evaluation 2.63 ± 0.24 ms 2.63 ± 0.22 ms 1
eval/Float32/evaluation_bumper 0.586 ± 0.014 ms 0.534 ± 0.013 ms 1.1
eval/Float32/evaluation_turbo 0.563 ± 0.032 ms 0.567 ± 0.03 ms 0.994
eval/Float32/evaluation_turbo_bumper 0.574 ± 0.014 ms 0.531 ± 0.013 ms 1.08
eval/Float64/derivative 14.3 ± 1.1 ms 14.4 ± 0.77 ms 0.993
eval/Float64/derivative_turbo 14.4 ± 1.1 ms 14.4 ± 1 ms 1
eval/Float64/evaluation 2.78 ± 0.24 ms 2.78 ± 0.24 ms 1
eval/Float64/evaluation_bumper 1.28 ± 0.044 ms 1.21 ± 0.043 ms 1.06
eval/Float64/evaluation_turbo 1.06 ± 0.067 ms 1.04 ± 0.06 ms 1.02
eval/Float64/evaluation_turbo_bumper 1.29 ± 0.044 ms 1.21 ± 0.042 ms 1.06
utils/combine_operators/break_sharing 0.0386 ± 0.00069 ms 0.039 ± 0.0007 ms 0.991
utils/convert/break_sharing 22.8 ± 1.1 μs 22.9 ± 0.98 μs 0.999
utils/convert/preserve_sharing 0.124 ± 0.0034 ms 0.124 ± 0.0034 ms 1
utils/copy/break_sharing 23.5 ± 0.91 μs 23.4 ± 0.94 μs 1
utils/copy/preserve_sharing 0.125 ± 0.0036 ms 0.126 ± 0.0033 ms 0.995
utils/count_constant_nodes/break_sharing 8.61 ± 0.12 μs 9.11 ± 0.23 μs 0.945
utils/count_constant_nodes/preserve_sharing 0.109 ± 0.0024 ms 0.11 ± 0.0028 ms 0.989
utils/count_depth/break_sharing 13.4 ± 0.39 μs 10.3 ± 0.15 μs 1.3
utils/count_nodes/break_sharing 8.57 ± 0.12 μs 8.44 ± 0.16 μs 1.02
utils/count_nodes/preserve_sharing 0.111 ± 0.0026 ms 0.11 ± 0.0029 ms 1
utils/get_set_constants!/break_sharing 0.0378 ± 0.001 ms 0.0344 ± 0.0011 ms 1.1
utils/get_set_constants!/preserve_sharing 0.228 ± 0.0063 ms 0.228 ± 0.006 ms 1
utils/get_set_constants_parametric 0.0481 ± 0.0022 ms 0.0475 ± 0.0018 ms 1.01
utils/has_constants/break_sharing 4.37 ± 0.064 μs 4.2 ± 0.075 μs 1.04
utils/has_operators/break_sharing 2.08 ± 0.033 μs 1.94 ± 0.022 μs 1.07
utils/hash/break_sharing 25.6 ± 0.43 μs 25.4 ± 0.42 μs 1.01
utils/hash/preserve_sharing 0.132 ± 0.0026 ms 0.132 ± 0.0027 ms 1
utils/index_constant_nodes/break_sharing 22.7 ± 0.94 μs 22.9 ± 0.83 μs 0.993
utils/index_constant_nodes/preserve_sharing 0.124 ± 0.0031 ms 0.125 ± 0.0029 ms 0.994
utils/is_constant/break_sharing 3.61 ± 0.071 μs 4.23 ± 0.079 μs 0.855
utils/simplify_tree/break_sharing 0.169 ± 0.0016 ms 0.171 ± 0.0015 ms 0.986
utils/simplify_tree/preserve_sharing 0.284 ± 0.0057 ms 0.334 ± 0.0061 ms 0.85
utils/string_tree/break_sharing 0.385 ± 0.015 ms 0.385 ± 0.014 ms 0.999
utils/string_tree/preserve_sharing 0.531 ± 0.025 ms 0.525 ± 0.02 ms 1.01
time_to_load 0.244 ± 0.001 s 0.243 ± 0.0024 s 1

@MilesCranmer
Copy link
Member

Nice!! Yeah this is exactly what I had in mind 😀

I think the remaining pieces needed are: (1) copying it over to the LoopVectorization and Bumper extensions, and (2) adding some more expression tests to probe different branches of code, as well as with turbo=true and bumper=true. Sorry it’s a bit tedious!! In the future I’d like to just have some macro that generates the LoopVectorization code in the extension.

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9741447211

Details

  • 24 of 24 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 95.52%

Totals Coverage Status
Change from base Build 9738548135: 0.006%
Covered Lines: 2196
Relevant Lines: 2299

💛 - Coveralls

@MilesCranmer
Copy link
Member

MilesCranmer commented Jul 4, 2024

FYI once this gets merged I may change things so that there is a single

Base.@kwdef struct EvaluationOptions{T,B,E}
    turbo::Val{T} = Val(false)
    bumper::Val{B} = Val(false)
    early_exit::Val{E} = Val(true)
end

which gets passed around. It should let eval_tree_array avoid keyword arguments (since use of keywords can affect some autodiff tools), and also simplifies the code a lot, as the functions wouldn't need to write Val(turbo) every time, etc.

If you're up for trying it in this PR, feel free, but I didn't want to have to ask you to do too much though, as I already appreciate this PR a lot 🙂

@nmheim
Copy link
Contributor Author

nmheim commented Jul 8, 2024

Hey! I took a quick stab at the EvaluationOptions you suggested. Currently they could be used like

expr(X; options=EvaluationOption(turbo=Val(true)))
# or like below (which I think introduces a type instability)
expr(X; options=EvaluationOption(turbo=true))

this breaks backwards compatibility, so I guess it would be nice to make things work with the current kwarg way of calling the expression?

also, we might also want to add throws_error to the eval options?

Happy to update this however you think is best!

test/test_evaluation.jl Outdated Show resolved Hide resolved
src/Evaluate.jl Outdated Show resolved Hide resolved
src/Evaluate.jl Outdated Show resolved Hide resolved
@MilesCranmer
Copy link
Member

MilesCranmer commented Jul 19, 2024

Also FYI I made a few changes that were easier to implement manually like (1) merging master and (2) changing options to eval_options (and the type to EvalOptions)

Re: throw_errors, I think we should leave it as-is in the method for generic evaluation.

docs/src/eval.md Outdated Show resolved Hide resolved
docs/src/eval.md Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 27, 2024

Pull Request Test Coverage Report for Build 10127877451

Details

  • 102 of 108 (94.44%) changed or added relevant lines in 4 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 96.006%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/precompile.jl 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
src/Evaluate.jl 4 97.73%
Totals Coverage Status
Change from base Build 10126963755: 0.1%
Covered Lines: 2428
Relevant Lines: 2529

💛 - Coveralls

@MilesCranmer
Copy link
Member

Thanks again for the great contribution!!

@MilesCranmer MilesCranmer self-requested a review July 28, 2024 01:06
@MilesCranmer MilesCranmer merged commit db39efc into SymbolicML:master Jul 28, 2024
10 checks passed
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.

3 participants