feat!: Decouple expression hashing and equality (#277)

* feat!: Decouple expression hashing and equality BREAKING: Expressions no longer use hashing for implementing equality BREAKING: Expression equality no longer takes commutativity into account In #276, @Shadow53 noted > One thing that may be useful enough to pull into its own PR is the > change to not use hashing in the implementation of `PartialEq` for > `Expression`, which also helps speed things up. We originally put this together in #27 to ensure that equality held in the face of commutativity, e.g., `1 + x == x + 1`. In addition to the performance benefits of decoupling the hashing and equality implementations, it makes sense to remove any special status for commutative operations in light of all the work we're doing on expression simplification. If we wished to ensure expressions are `Eq` if and only if they represent the same mathematical expression, we'd have to have equality contingent upon simplification, which would be even more costly. * fix: Use Self::
rigetti · Aug 10, 2023 · fd27679 · fd27679
1 parent 02c59f3
commit fd27679
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 113 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/quil-rs/src/expression/mod.rs b/quil-rs/src/expression/mod.rs
@@ -13,10 +13,12 @@
 // limitations under the License.
 
 use crate::{
-    hash::{hash_f64, hash_to_u64},
+    hash::hash_f64,
+    imag,
+    instruction::MemoryReference,
     parser::{lex, parse_expression, ParseError},
     program::{disallow_leftover, ParseProgramError},
-    {imag, instruction::MemoryReference, real},
+    real,
 };
 use lexical::{format, to_string_with_options, WriteFloatOptions};
 use nom_locate::LocatedSpan;
@@ -53,13 +55,13 @@ pub enum Expression {
     Address(MemoryReference),
     FunctionCall(FunctionCallExpression),
     Infix(InfixExpression),
-    Number(num_complex::Complex64),
+    Number(Complex64),
     PiConstant,
     Prefix(PrefixExpression),
     Variable(String),
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct FunctionCallExpression {
     pub function: ExpressionFunction,
     pub expression: Box<Expression>,
@@ -74,7 +76,7 @@ impl FunctionCallExpression {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct InfixExpression {
     pub left: Box<Expression>,
     pub operator: InfixOperator,
@@ -91,7 +93,7 @@ impl InfixExpression {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct PrefixExpression {
     pub operator: PrefixOperator,
     pub expression: Box<Expression>,
@@ -106,86 +108,77 @@ impl PrefixExpression {
     }
 }
 
+impl PartialEq for Expression {
+    // Implemented by hand since we can't derive with f64s hidden inside.
+    fn eq(&self, other: &Self) -> bool {
+        match (self, other) {
+            (Self::Address(left), Self::Address(right)) => left == right,
+            (Self::Infix(left), Self::Infix(right)) => left == right,
+            (Self::Number(left), Self::Number(right)) => left == right,
+            (Self::Prefix(left), Self::Prefix(right)) => left == right,
+            (Self::FunctionCall(left), Self::FunctionCall(right)) => left == right,
+            (Self::Variable(left), Self::Variable(right)) => left == right,
+            (Self::PiConstant, Self::PiConstant) => true,
+            _ => false,
+        }
+    }
+}
+
+// Implemented by hand since we can't derive with f64s hidden inside.
+impl Eq for Expression {}
+
 impl Hash for Expression {
     // Implemented by hand since we can't derive with f64s hidden inside.
-    // Also to understand when things should be the same, like with commutativity (`1 + 2 == 2 + 1`).
-    // See https://github.com/rigetti/quil-rust/issues/27
     fn hash<H: Hasher>(&self, state: &mut H) {
-        use std::cmp::{max_by_key, min_by_key};
-        use Expression::*;
         match self {
-            Address(m) => {
+            Self::Address(m) => {
                 "Address".hash(state);
                 m.hash(state);
             }
-            FunctionCall(FunctionCallExpression {
+            Self::FunctionCall(FunctionCallExpression {
                 function,
                 expression,
             }) => {
                 "FunctionCall".hash(state);
                 function.hash(state);
                 expression.hash(state);
             }
-            Infix(InfixExpression {
+            Self::Infix(InfixExpression {
                 left,
                 operator,
                 right,
             }) => {
                 "Infix".hash(state);
                 operator.hash(state);
-                match operator {
-                    InfixOperator::Plus | InfixOperator::Star => {
-                        // commutative, so put left & right in decreasing order by hash value
-                        let (a, b) = (
-                            min_by_key(&left, &right, hash_to_u64),
-                            max_by_key(&left, &right, hash_to_u64),
-                        );
-                        a.hash(state);
-                        b.hash(state);
-                    }
-                    _ => {
-                        left.hash(state);
-                        right.hash(state);
-                    }
-                }
+                left.hash(state);
+                right.hash(state);
             }
-            Number(n) => {
+            Self::Number(n) => {
                 "Number".hash(state);
                 // Skip zero values (akin to `format_complex`).
-                // Also, since f64 isn't hashable, use the u64 binary representation.
-                // The docs claim this is rather portable: https://doc.rust-lang.org/std/primitive.f64.html#method.to_bits
                 if n.re.abs() > 0f64 {
                     hash_f64(n.re, state)
                 }
                 if n.im.abs() > 0f64 {
                     hash_f64(n.im, state)
                 }
             }
-            PiConstant => {
+            Self::PiConstant => {
                 "PiConstant".hash(state);
             }
-            Prefix(p) => {
+            Self::Prefix(p) => {
                 "Prefix".hash(state);
                 p.operator.hash(state);
                 p.expression.hash(state);
             }
-            Variable(v) => {
+            Self::Variable(v) => {
                 "Variable".hash(state);
                 v.hash(state);
             }
         }
     }
 }
 
-impl PartialEq for Expression {
-    // Partial equality by hash value
-    fn eq(&self, other: &Self) -> bool {
-        hash_to_u64(self) == hash_to_u64(other)
-    }
-}
-
-impl Eq for Expression {}
-
 macro_rules! impl_expr_op {
     ($name:ident, $name_assign:ident, $function:ident, $function_assign:ident, $operator:ident) => {
         impl $name for Expression {
@@ -215,11 +208,7 @@ impl_expr_op!(Mul, MulAssign, mul, mul_assign, Star);
 impl_expr_op!(Div, DivAssign, div, div_assign, Slash);
 
 /// Compute the result of an infix expression where both operands are complex.
-fn calculate_infix(
-    left: &num_complex::Complex64,
-    operator: &InfixOperator,
-    right: &num_complex::Complex64,
-) -> num_complex::Complex64 {
+fn calculate_infix(left: &Complex64, operator: &InfixOperator, right: &Complex64) -> Complex64 {
     use InfixOperator::*;
     match operator {
         Caret => left.powc(*right),
@@ -231,10 +220,7 @@ fn calculate_infix(
 }
 
 /// Compute the result of a Quil-defined expression function where the operand is complex.
-fn calculate_function(
-    function: &ExpressionFunction,
-    argument: &num_complex::Complex64,
-) -> num_complex::Complex64 {
+fn calculate_function(function: &ExpressionFunction, argument: &Complex64) -> Complex64 {
     use ExpressionFunction::*;
     match function {
         Sine => argument.sin(),
@@ -323,9 +309,9 @@ impl Expression {
     /// ```
     pub fn evaluate(
         &self,
-        variables: &HashMap<String, num_complex::Complex64>,
+        variables: &HashMap<String, Complex64>,
         memory_references: &HashMap<&str, Vec<f64>>,
-    ) -> Result<num_complex::Complex64, EvaluationError> {
+    ) -> Result<Complex64, EvaluationError> {
         use Expression::*;
 
         match self {
@@ -660,18 +646,11 @@ impl fmt::Display for InfixOperator {
 
 #[cfg(test)]
 mod tests {
-    use std::collections::{hash_map::DefaultHasher, HashSet};
-
-    use num_complex::Complex64;
-    use proptest::prelude::*;
-
-    use crate::{
-        expression::{EvaluationError, Expression, ExpressionFunction},
-        real,
-        reserved::ReservedToken,
-    };
-
     use super::*;
+    use crate::hash::hash_to_u64;
+    use crate::reserved::ReservedToken;
+    use proptest::prelude::*;
+    use std::collections::HashSet;
 
     #[test]
     fn simplify_and_evaluate() {
@@ -862,21 +841,6 @@ mod tests {
             prop_assert_ne!(&first, &differing);
         }
 
-        #[test]
-        fn eq_commutative(a in any::<f64>(), b in any::<f64>()) {
-            let first = Expression::Infix(InfixExpression {
-                left: Box::new(Expression::Number(real!(a))),
-                operator: InfixOperator::Plus,
-                right: Box::new(Expression::Number(real!(b))),
-            } );
-            let second = Expression::Infix(InfixExpression {
-                left: Box::new(Expression::Number(real!(b))),
-                operator: InfixOperator::Plus,
-                right: Box::new(Expression::Number(real!(a))),
-            });
-            prop_assert_eq!(first, second);
-        }
-
         #[test]
         fn hash(a in any::<f64>(), b in any::<f64>()) {
             let first = Expression::Infix (InfixExpression {
@@ -892,36 +856,9 @@ mod tests {
             assert!(!set.contains(&differing))
         }
 
-        #[test]
-        fn hash_commutative(a in any::<f64>(), b in any::<f64>()) {
-            let first = Expression::Infix(InfixExpression {
-                left: Box::new(Expression::Number(real!(a))),
-                operator: InfixOperator::Plus,
-                right: Box::new(Expression::Number(real!(b))),
-            } );
-            let second = Expression::Infix(InfixExpression {
-                left: Box::new(Expression::Number(real!(b))),
-                operator: InfixOperator::Plus,
-                right: Box::new(Expression::Number(real!(a))),
-            } );
-            let mut set = HashSet::new();
-            set.insert(first);
-            assert!(set.contains(&second));
-        }
-
         #[test]
         fn eq_iff_hash_eq(x in arb_expr(), y in arb_expr()) {
-            let h_x = {
-                let mut s = DefaultHasher::new();
-                x.hash(&mut s);
-                s.finish()
-            };
-            let h_y = {
-                let mut s = DefaultHasher::new();
-                y.hash(&mut s);
-                s.finish()
-            };
-            prop_assert_eq!(x == y, h_x == h_y);
+            prop_assert_eq!(x == y, hash_to_u64(&x) == hash_to_u64(&y));
         }
 
         #[test]

diff --git a/quil-rs/src/expression/simplification.rs b/quil-rs/src/expression/simplification.rs
@@ -1,11 +1,11 @@
 /// Complex machinery for simplifying [`Expression`]s.
 use crate::{
     expression::{
-        format_complex, hash_to_u64, imag, is_small, real, Expression, ExpressionFunction,
-        FunctionCallExpression, InfixExpression, InfixOperator, MemoryReference, PrefixExpression,
-        PrefixOperator,
+        format_complex, is_small, Expression, ExpressionFunction, FunctionCallExpression,
+        InfixExpression, InfixOperator, MemoryReference, PrefixExpression, PrefixOperator,
     },
-    hash::hash_f64,
+    hash::{hash_f64, hash_to_u64},
+    imag, real,
 };
 use egg::{define_language, rewrite as rw, Id, Language, RecExpr};
 use once_cell::sync::Lazy;