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

Fix: Correct Scope of Symbolic Variable in where Rule Test #643

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

bowenszhu
Copy link
Member

@bowenszhu bowenszhu commented Aug 30, 2024

The original test for the where clause in rules incorrectly handled the scope of the symbolic variable a. Two distinct symbolic variables with the same name were created, leading to unexpected behavior when checking for object equality in the _f function.

This PR moves the definition of _f inside the @testset block, ensuring that it references the same symbolic variable a as the rule definition. This correction aligns the test with the intended behavior of the where clause.

@bowenszhu bowenszhu changed the title Fix: Correct Scope of Symbol in where Rule Test Fix: Correct Scope of Symbolic Variable in where Rule Test Aug 30, 2024
Copy link
Contributor

Benchmark Results

master fc75095... master/fc7509587c2550...
overhead/acrule/a+2 0.768 ± 0.016 μs 0.726 ± 0.02 μs 1.06
overhead/acrule/a+2+b 0.735 ± 0.013 μs 0.703 ± 0.019 μs 1.04
overhead/acrule/a+b 0.254 ± 0.0089 μs 0.253 ± 0.012 μs 1
overhead/acrule/noop:Int 25.9 ± 0.05 ns 25.3 ± 0.05 ns 1.02
overhead/acrule/noop:Sym 0.0366 ± 0.0056 μs 0.0365 ± 0.0053 μs 1
overhead/rule/noop:Int 0.0438 ± 0.0011 μs 0.0451 ± 0.0011 μs 0.97
overhead/rule/noop:Sym 0.055 ± 0.0033 μs 0.0588 ± 0.0023 μs 0.936
overhead/rule/noop:Term 0.0547 ± 0.003 μs 0.0585 ± 0.0021 μs 0.935
overhead/ruleset/noop:Int 0.126 ± 0.0032 μs 0.127 ± 0.0031 μs 0.991
overhead/ruleset/noop:Sym 0.147 ± 0.0044 μs 0.149 ± 0.0053 μs 0.987
overhead/ruleset/noop:Term 3.02 ± 0.1 μs 3.06 ± 0.11 μs 0.987
overhead/simplify/noop:Int 0.167 ± 0.0012 μs 0.149 ± 0.0021 μs 1.12
overhead/simplify/noop:Sym 0.184 ± 0.0077 μs 0.17 ± 0.0041 μs 1.08
overhead/simplify/noop:Term 0.0356 ± 0.0016 ms 0.0348 ± 0.0016 ms 1.02
overhead/simplify/randterm (+, *):serial 0.0848 ± 0.001 s 0.0851 ± 0.00089 s 0.997
overhead/simplify/randterm (+, *):thread 0.051 ± 0.03 s 0.0509 ± 0.029 s 1
overhead/simplify/randterm (/, *):serial 0.212 ± 0.0076 ms 0.205 ± 0.007 ms 1.03
overhead/simplify/randterm (/, *):thread 0.239 ± 0.0091 ms 0.234 ± 0.0084 ms 1.02
overhead/substitute/a 0.0588 ± 0.0015 ms 0.0603 ± 0.0017 ms 0.975
overhead/substitute/a,b 0.052 ± 0.0015 ms 0.0521 ± 0.0014 ms 0.997
overhead/substitute/a,b,c 16.7 ± 0.6 μs 17.2 ± 0.6 μs 0.97
polyform/easy_iszero 28.6 ± 1.9 μs 28.8 ± 2.1 μs 0.993
polyform/isone 3.1 ± 0.01 ns 2.79 ± 0.01 ns 1.11
polyform/iszero 1.1 ± 0.029 ms 1.09 ± 0.028 ms 1.01
polyform/simplify_fractions 1.61 ± 0.04 ms 1.6 ± 0.04 ms 1.01
time_to_load 2.11 ± 0.0054 s 2.12 ± 0.013 s 0.999

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@bowenszhu bowenszhu merged commit 3303acf into master Aug 30, 2024
8 of 12 checks passed
@bowenszhu bowenszhu deleted the fix/where-rule-scope branch August 30, 2024 04:17
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.

1 participant