rigetti
`NaN` hashing/ordering is invalid
The documentation for Expressions states often:
Note that when comparing Quil expressions, any embedded NaNs are treated as equal to other NaNs, not unequal, in contravention of the IEEE 754 spec [sic][^1]
[^1]: To be pedantic, IEEE 754 specifies that NaN comparisons are unordered, not unequal.
The PartialEq implementation for Expression indeed considers two Numbers equal if, for example, their real and imaginary parts are both NaN; however, the Hash implementation hashes the bit patterns of those NaNs, so if they are different NaNs, for example, quiet vs signaling forms, they will hash to different values.
This is a problem because a valid Hash implementation requires that $x = y \implies Hash(x) = Hash(y)$.
We could instead use ordered_float or adjust our implementation to similarly treat all NaNs as a single specific value, though as those docs state, it has implications for interning: we'll lose semantic information about different NaN values.
Note that this is related to #457. We should fix this, and doing so will impact Expression simplification, and the solution may depend on how we want to simplify expressions involving floating point operations.
My 2¢ here:
- Using
OrderedFloatends up being a headache because you lose literals; I'd rather just implement the logic ourselves. - However, one approach to implementing equality/hashing is by converting to
OrderedFloats and comparing/hashing those – using them as temporaries only. - I think it's 100% fine to elide the distinction between NaNs that enter expressions; we actively want to do that.
- Point 3 is especially true because Rust guarantees that SNaNs cannot appear unless they're provided explicitly, so we can collapse NaNs down.
- I think the solution to this is independent of the solution to #457, since we want to treat all NaNs as equivalent.
- (In the same spirit as your pedantry: unordered implies unequal, so equal implies ordered, so the comment returns to being pedantically correct! 😜)
