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.

Sign up for GitHub

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

feat!: Decouple expression hashing and equality #277

Merged
merged 3 commits into from
Aug 10, 2023
+50 −113
Merged

feat!: Decouple expression hashing and equality #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c43c8f9
feat!: Decouple expression hashing and equality
Aug 10, 2023
a0dc014
fix: Use Self::
Aug 10, 2023
1186f1d
Merge branch 'main' into equality-and-hashing
Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

153 changes: 45 additions & 108 deletions quil-rs/src/expression/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>,
Expand All @@ -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,
Expand All @@ -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>,
Expand All @@ -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 {
Expand Down Expand Up @@ -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),
Expand All @@ -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(),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Expand All @@ -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]
Expand Down
8 changes: 4 additions & 4 deletions quil-rs/src/expression/simplification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down