Simplify eggspressions

[genos]

This is a DRAFT of using uses egg to tackle #208.

TODO:

• Incorporate egg in a private module to do Expression::simplify
• Pass current test suite + more
• Handle the "(neg <number>)" edge case in simplification, not with if let Ok(number) = … workaround†
• Actual error handling, no panic! or expect!
• Documentation

†I haven't figured out how to manage this one; any help from reviewers would be much appreciated. Fixed in d4f7fbf

[kalzoo]

This is rad, @genos !! Sure, it is a little funny to simplify via stringification, but it's a lot cleaner than having to rewrite all that logic using the enums directly. Two things:

• I'd be interested in simple benchmarks to track performance. I'm not too worried about regression here but would be good to know, given how often it's invoked elsewhere.
• (see other comment)

[genos]

Thanks, @kalzoo! Re:

• I'd be interested in simple benchmarks to track performance. I'm not too worried about regression here but would be good to know, given how often it's invoked elsewhere.

What did you have in mind? A criterion bench with a randomly-generated-of-a-fixed-sized Expression to simplify?

[kalzoo]

Thanks, @kalzoo! Re:

• I'd be interested in simple benchmarks to track performance. I'm not too worried about regression here but would be good to know, given how often it's invoked elsewhere.

What did you have in mind? A criterion bench with a randomly-generated-of-a-fixed-sized Expression to simplify?

That could work, so long as it's only randomly generated once and then committed - maybe a few of them for good measure. We'd want benchmark inputs to be static so that they can provide meaningful feedback to development.

[genos]

Sure, it is a little funny to simplify via stringification,

This would be easier if egg exposed some of the internals of RecExpr, specifically parsing from Sexps. I think I'll steal this, actually, rather than rely on the stringification.

Edit: see 6b83e53

[genos]

What did you have in mind? A criterion bench with a randomly-generated-of-a-fixed-sized Expression to simplify?

That could work, so long as it's only randomly generated once and then committed - maybe a few of them for good measure. We'd want benchmark inputs to be static so that they can provide meaningful feedback to development.

Actually, why not just reuse the .quil files we've already got? The benches/parser.rs benchmark reuses good .quil programs from quilc. They're full Quil programs, not just expressions, but could prove useful and relatively low lift to use.

[genos]

@kalzoo re: generating random expressions to benchmark simplifying, I've tried adding rand as a dependency and the following main.rs to generate a static file of them, but it looks like the Expression ⇒ String ⇒ Expression roundtrip doesn't quite work:

thread 'main' panicked at
'pi/cis(sin(sin(-%owlhhqxriv^cos(%mghfxnbixn)))) => error while parsing:
at line 1, column 28 (OPERATOR(^)): expected RParenthesis, found OPERATOR(^)', src/main.rs:83:23

main.rs

use quil_rs::{
    expression::{Expression, ExpressionFunction, InfixOperator, PrefixOperator},
    instruction::MemoryReference,
};
use rand::{rngs::StdRng, Rng, SeedableRng};
use std::{fs::File, io::Write, str::FromStr};

// rand::dists::Alphanumeric isn't what we need
fn rand_string(rng: &mut impl Rng) -> String {
    (0..10).map(|_| rng.gen_range('a'..'z')).collect()
}

fn rand_expr(rng: &mut impl Rng, max_depth: usize) -> Expression {
    if max_depth == 0 {
        Expression::PiConstant
    } else {
        match rng.gen_range(0..7) {
            0 => {
                let name = rand_string(rng);
                let index = rng.gen();
                Expression::Address(MemoryReference { name, index })
            }
            1 => {
                let function = match rng.gen_range(0..5) {
                    0 => ExpressionFunction::Cis,
                    1 => ExpressionFunction::Cosine,
                    2 => ExpressionFunction::Exponent,
                    3 => ExpressionFunction::Sine,
                    _ => ExpressionFunction::SquareRoot,
                };
                let expression = Box::new(rand_expr(rng, max_depth / 2));
                Expression::FunctionCall {
                    function,
                    expression,
                }
            }
            2 => {
                let left = Box::new(rand_expr(rng, max_depth / 2));
                let operator = match rng.gen_range(0..5) {
                    0 => InfixOperator::Caret,
                    1 => InfixOperator::Plus,
                    2 => InfixOperator::Minus,
                    3 => InfixOperator::Slash,
                    _ => InfixOperator::Star,
                };
                let right = Box::new(rand_expr(rng, max_depth / 2));
                Expression::Infix {
                    left,
                    operator,
                    right,
                }
            }
            3 => {
                let re = rng.gen::<f64>();
                let im = rng.gen::<f64>();
                Expression::Number(num_complex::Complex64 { re, im })
            }
            4 => Expression::PiConstant,
            5 => {
                let operator = if rng.gen() {
                    PrefixOperator::Plus
                } else {
                    PrefixOperator::Minus
                };
                let expression = Box::new(rand_expr(rng, max_depth / 2));
                Expression::Prefix {
                    operator,
                    expression,
                }
            }
            _ => Expression::Variable(rand_string(rng)),
        }
    }
}

fn main() {
    let mut rng = StdRng::seed_from_u64(8675309);
    let mut out = File::create("expressions.txt").expect("We should be able to create a file.");
    for _ in 0..1_000 {
        let expression = rand_expr(&mut rng, 256);
        match Expression::from_str(&expression.to_string()) {
            Ok(_) => write!(out, "{}", expression).expect("We should be able to write to the file"),
            Err(e) => panic!("{expression} => {e}"),
        }
    }
}

[genos]

Further proptest play lead to another expression that fails to round trip: "cis(AA[0]^a0[428])"

[genos]

@kalzoo (& friends) I finally managed the negation edge case! Turns out I was relying on Expression::from_str for turning back from a symbolic expression into an expression, and that parses "-1" as Prefix{Minus, Num{1, 0}} instead of Num{-1, 0}. d4f7fbf fixes this.

[genos]

Ok! This now passes all my local testing and is about where I think it should be. Pinging folks for re-reviews.

In order to get round-tripping to go (#215), I've added the fn parenthesized(expression: &Expression) -> String in the testing module. Unused elsewhere, but it ensures unambiguous behavior for our tests.

[genos]

Even simpler now; I removed the ordered-float dependency that I originally added, plus made the custom Complex type just a wrapper for num_complex::Complex64 implementing the various traits we need.

[kalzoo]

Small comments - I think it's just about good to go! We can add benchmarks with a follow-on PR - it'll be good to have some focused on simplification.

[kalzoo]

good simplification in c3f363f, @genos

can you catch up to main and then we can merge?

[genos]

good simplification in c3f363f, @genos

can you catch up to main and then we can merge?

Thanks @kalzoo! Fixed merge conflicts with main, but the Deny task is still failing. It looks like proptest and criterion are pulling two different versions of regex_syntax. Any thoughts on fixing that?

[genos]

Perhaps related: we still have Cargo.lock in our .gitignore, but it's in the repo.

[kalzoo]

the Deny task is still failing. It looks like proptest and criterion are pulling two different versions of regex_syntax. Any thoughts on fixing that?

I'm not too worried about that, both of the warnings are safely ignorable at the moment, even if they add noise to CI results. We can fix them off main.

Perhaps related: we still have Cargo.lock in our .gitignore, but it's in the repo.

Ah good point, can you rm that here?

nvm, was quick to get

[genos]

🚨 Squashing and merging 🚨

[kalzoo]

🚨 Squashing and merging 🚨

Thanks for your great work on this @genos ! This is both more thorough and more maintainable than we would have made any simplification rules by hand. And now we're using egg in production!