Open amilsted opened 1 year ago
master | 40c837f93cb3dc... | t[master]/t[40c837f93cb3dc...] | |
---|---|---|---|
overhead/acrule/a+2 | 1.03 ± 0.22 μs | 1.31 ± 0.51 μs | 0.787 |
overhead/acrule/a+2+b | 1.08 ± 0.33 μs | 1.24 ± 0.42 μs | 0.869 |
overhead/acrule/a+b | 0.352 ± 0.056 μs | 0.487 ± 0.11 μs | 0.723 |
overhead/acrule/noop:Int | 22.9 ± 2.1 ns | 25.8 ± 2.5 ns | 0.89 |
overhead/acrule/noop:Sym | 0.0467 ± 0.0096 μs | 0.0496 ± 0.0096 μs | 0.941 |
overhead/rule/noop:Int | 0.0366 ± 0.0047 μs | 0.0474 ± 0.015 μs | 0.771 |
overhead/rule/noop:Sym | 0.0666 ± 0.013 μs | 0.0824 ± 0.022 μs | 0.809 |
overhead/rule/noop:Term | 0.0678 ± 0.014 μs | 0.0773 ± 0.02 μs | 0.877 |
overhead/ruleset/noop:Int | 0.143 ± 0.02 μs | 0.162 ± 0.032 μs | 0.884 |
overhead/ruleset/noop:Sym | 0.177 ± 0.025 μs | 0.212 ± 0.041 μs | 0.836 |
overhead/ruleset/noop:Term | 5.11 ± 0.89 μs | 5.5 ± 0.9 μs | 0.93 |
overhead/simplify/noop:Int | 0.191 ± 0.027 μs | 0.238 ± 0.054 μs | 0.803 |
overhead/simplify/noop:Sym | 0.223 ± 0.029 μs | 0.307 ± 0.073 μs | 0.725 |
overhead/simplify/noop:Term | 0.0731 ± 0.01 ms | 0.104 ± 0.021 ms | 0.702 |
overhead/simplify/randterm (+, *):serial | 0.185 ± 0.0095 s | 0.223 ± 0.012 s | 0.829 |
overhead/simplify/randterm (+, *):thread | 0.114 ± 0.015 s | 0.145 ± 0.02 s | 0.787 |
overhead/simplify/randterm (/, *):serial | 0.407 ± 0.062 ms | 0.54 ± 0.11 ms | 0.753 |
overhead/simplify/randterm (/, *):thread | 0.472 ± 0.064 ms | 0.613 ± 0.11 ms | 0.769 |
overhead/substitute/a | 0.109 ± 0.023 ms | 0.108 ± 0.019 ms | 1.01 |
overhead/substitute/a,b | 0.0927 ± 0.015 ms | 0.104 ± 0.02 ms | 0.888 |
overhead/substitute/a,b,c | 26.4 ± 4.4 μs | 29.6 ± 5.4 μs | 0.892 |
polyform/easy_iszero | 0.0617 ± 0.0081 ms | 0.0607 ± 0.011 ms | 1.02 |
polyform/isone | 2.6 ± 0.2 ns | 3 ± 0.2 ns | 0.867 |
polyform/iszero | 2.15 ± 0.36 ms | 2.3 ± 0.4 ms | 0.937 |
polyform/simplify_fractions | 2.95 ± 0.43 ms | 3.01 ± 0.39 ms | 0.981 |
time_to_load | 5.72 ± 0.23 s | 5.59 ± 0.12 s | 1.02 |
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).
Fine with this, could you add a test or two?
Merging #530 (8171356) into master (e4519eb) will not change coverage. The diff coverage is
n/a
.
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
@@ Coverage Diff @@
## master #530 +/- ##
=======================================
Coverage 81.19% 81.19%
=======================================
Files 15 15
Lines 1856 1856
=======================================
Hits 1507 1507
Misses 349 349
Impacted Files | Coverage Δ | |
---|---|---|
src/simplify_rules.jl | 93.75% <ø> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Fine with this, could you add a test or two?
Done. Also added a couple more rules (correct ones this time 😅).
Just curious @shashi is there an ::_iscomplex
?
@MilesCranmer No I don't think so, but it could be added.
Looks like the tests I added are passing now. I guess the segfault is a more general issue?
@shashi The reason I asked about the ::_iscomplex
is that it looks like adding this rule would hurt the performance of simplification by about 20% across the board (https://github.com/JuliaSymbolics/SymbolicUtils.jl/pull/530#issuecomment-1608440859) even though I think some of those tests are real numbers only. So maybe having that _iscomplex
check would reduce the impact of this on non-complex simplification problems?
(It could just be measurement uncertainty in the benchmarks though?? Not sure. Probably worth a local benchmark to be sure)
Not sure if this is the right thing to do, but a PR seemed like the easiest way to bring this up.