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

new(rs): fully by-hand expression simplification #273

Merged
merged 33 commits into from
Sep 13, 2023
Merged

Conversation

genos
Copy link
Contributor

@genos genos commented Aug 4, 2023

This combines the work in #267 and #272; putting together multiple passes of by_hand(by_egg(expression)) yields dramatically simpler results.

cargo test --release on the big expression in #259 yielded

…<SNIP>…
yielded `-14553.919984548369+(gamma[0]*-0.6379644761225252)` instead of the expected `-0.637964476122525*gamma[0] - 14553.9199845484`', quil-rs/src/expression/simplification/mod.rs:173:5

which looks pretty darn close.

This is just a proof of concept right now. Paying for multiple roundtrips to & from a format egg likes is inefficient.

This now relies solely on by-hand simplification, and does rather well:

  • one test fails (affine, expecting ax + b + cx + d to simplify to (a + c)x + (b + d), I can't quite get around a syntax error in trying to cover this case;
  • one test hangs (the big one from Hashing Expressions should be fast #259);

The by-hand simplification has a maximum limit of times it's allowed to run, to avoid some "test XXX has overflowed its stack" issues stemming from infinite recursions.

@genos genos marked this pull request as draft August 4, 2023 01:47
@genos genos requested review from Shadow53 and kalzoo August 4, 2023 01:47
@genos genos mentioned this pull request Aug 31, 2023
@genos genos changed the title new(rs): combine egg & by hand to simplify expressions new(rs): fully by-hand expression simplification Aug 31, 2023
@genos genos marked this pull request as ready for review August 31, 2023 19:41
@genos
Copy link
Contributor Author

genos commented Aug 31, 2023

Using our criterion benchmarks for simplification, we can see that this is a definite speed win (at least on these examples):

stat ci_lb ci_ub estimate standard_error
0 mean -0.976248 -0.976095 -0.976172 3.90988e-05
1 median -0.976067 -0.975995 -0.976021 1.83028e-05
2 mean -0.869604 -0.869046 -0.869322 0.000143586
3 median -0.869341 -0.869042 -0.869128 7.87167e-05
4 mean -0.902095 -0.901426 -0.901724 0.000171065
5 median -0.901751 -0.901554 -0.901635 5.39497e-05
6 mean -0.948414 -0.94809 -0.948243 8.21166e-05
7 median -0.948482 -0.948219 -0.948399 6.70503e-05
8 mean -0.955853 -0.955561 -0.955701 7.3724e-05
9 median -0.955898 -0.955611 -0.955759 8.2507e-05
10 mean -0.974691 -0.974514 -0.9746 4.49849e-05
11 median -0.974746 -0.974502 -0.974659 7.3237e-05
12 mean -0.971812 -0.971635 -0.971717 4.53122e-05
13 median -0.971848 -0.971603 -0.971685 7.07869e-05
14 mean -0.963308 -0.962992 -0.963153 8.16875e-05
15 median -0.963465 -0.963165 -0.963188 6.00217e-05
16 mean -0.447311 -0.441571 -0.444321 0.00145615
17 median -0.451319 -0.444091 -0.447164 0.0016764
18 mean -0.961768 -0.961548 -0.961659 5.59687e-05
19 median -0.961753 -0.961533 -0.961616 4.78361e-05
20 mean -0.945897 -0.945562 -0.945719 8.51306e-05
21 median -0.945692 -0.945606 -0.945644 2.18483e-05
22 mean -0.963264 -0.963018 -0.963133 6.2732e-05
23 median -0.963295 -0.963043 -0.963225 5.97133e-05
24 mean -0.922564 -0.921984 -0.922254 0.000148132
25 median -0.922672 -0.92213 -0.92255 0.000121146
26 mean -0.946045 -0.945716 -0.945865 8.3576e-05
27 median -0.945903 -0.945634 -0.945763 6.78746e-05
28 mean -0.922287 -0.921802 -0.922018 0.000123883
29 median -0.922128 -0.921868 -0.921989 6.92781e-05
30 mean -0.961257 -0.960982 -0.961115 6.98006e-05
31 median -0.961451 -0.961175 -0.961389 6.77671e-05
32 mean -0.963683 -0.963486 -0.963577 5.01635e-05
33 median -0.963689 -0.963544 -0.963622 3.52981e-05
34 mean -0.96557 -0.965348 -0.965449 5.64387e-05
35 median -0.965529 -0.965331 -0.965412 5.0992e-05
36 mean -0.634816 -0.631642 -0.633185 0.000817713
37 median -0.633496 -0.632387 -0.632849 0.000268155
38 mean -0.959962 -0.959735 -0.959845 5.76279e-05
39 median -0.95997 -0.959802 -0.959903 4.14346e-05
40 mean -0.961065 -0.960856 -0.960958 5.3078e-05
41 median -0.961012 -0.960923 -0.96098 2.02521e-05
42 mean -0.887373 -0.886575 -0.886952 0.000203981
43 median -0.887485 -0.887204 -0.887308 6.57965e-05
44 mean -0.947567 -0.947232 -0.947389 8.62616e-05
45 median -0.947191 -0.947063 -0.947123 3.32004e-05
46 mean -0.852976 -0.851584 -0.852389 0.000364723
47 median -0.853305 -0.852109 -0.852762 0.000326995
48 mean -0.937443 -0.937166 -0.937308 6.96309e-05
49 median -0.937442 -0.937284 -0.937373 3.83955e-05

@genos
Copy link
Contributor Author

genos commented Aug 31, 2023

A plot is worth 1k words

median

@genos
Copy link
Contributor Author

genos commented Aug 31, 2023

Oh, and the affine test now passes, sorry!

Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

Looks good! Got some suggestions. With the simplifications, it is possible to condense multiple cases that do the same thing into a single branch -- I made some suggestion to illustrate, but did not make a suggestion for everything.

quil-rs/src/expression/simplification/mod.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
@genos
Copy link
Contributor Author

genos commented Sep 5, 2023

Thanks @Shadow53! c2b60de tries to respond to your excellent review comments.

@genos
Copy link
Contributor Author

genos commented Sep 7, 2023

Thanks @mac01021! 3b1957f addresses your other points; I'll put up another commit with more unit testing later.

Edit: 60ffad9 improves test coverage.

Copy link
Contributor

@Shadow53 Shadow53 left a comment

Choose a reason for hiding this comment

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

  • Found some more places where match cases could be combined
    • Pointing these out to avoid code duplicate
  • Identified what seem to be redundant calls to simplify()
  • One possible logic error

quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
);
let new = simplify_infix(
multiplier,
// Addition associative, right: a + (b + c) = (a + b) + c, pick the shorter
Copy link
Contributor

Choose a reason for hiding this comment

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

Could merge this with the next case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as before re: reconstructing the original

quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
min_by_key(original, new, size)
}

// Multiplication associative, right: a * (b * c) = (a * b) * c, pick the shorter
Copy link
Contributor

Choose a reason for hiding this comment

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

Could merge with the next case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as before re: reconstructing the original

min_by_key(original, new, size)
}

// Subtraction "associative" (not really), right: a - (b - c) = (a + c) - b
Copy link
Contributor

Choose a reason for hiding this comment

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

Could merge with the below case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as before re: reconstructing the original

min_by_key(original, new, size)
}

// Right distribution: a * (b + c) = (a * b) + (a * c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could merge with the next case, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as before re: reconstructing the original

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

wow. too bad that egg didn't work out at this point, but good to know that all of this was necessary before doing it!

My approval is pending only validation with internal software.

quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
quil-rs/src/expression/simplification/by_hand.rs Outdated Show resolved Hide resolved
@genos genos merged commit 5a80c38 into main Sep 13, 2023
8 checks passed
@genos genos deleted the combo-simplifier branch September 13, 2023 13:34
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.

4 participants