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 x + conj(x) rule #530

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

amilsted
Copy link

Not sure if this is the right thing to do, but a PR seemed like the easiest way to bring this up.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2023

Benchmark Results

master 40c837f... t[master]/t[40c837f...]
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

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).

@shashi
Copy link
Member

shashi commented Jun 29, 2023

Fine with this, could you add a test or two?

@codecov-commenter
Copy link

Codecov Report

Merging #530 (8171356) into master (e4519eb) will not change coverage.
The diff coverage is n/a.

❗ 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% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amilsted
Copy link
Author

amilsted commented Jul 5, 2023

Fine with this, could you add a test or two?

Done. Also added a couple more rules (correct ones this time 😅).

@MilesCranmer
Copy link
Contributor

Just curious @shashi is there an ::_iscomplex?

@shashi
Copy link
Member

shashi commented Jul 22, 2023

@MilesCranmer No I don't think so, but it could be added.

@amilsted
Copy link
Author

amilsted commented Sep 1, 2023

Looks like the tests I added are passing now. I guess the segfault is a more general issue?

@MilesCranmer
Copy link
Contributor

@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 (#530 (comment)) 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?

@MilesCranmer
Copy link
Contributor

(It could just be measurement uncertainty in the benchmarks though?? Not sure. Probably worth a local benchmark to be sure)

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.

None yet

4 participants