Skip to content

Conversation

@kalzoo
Copy link
Contributor

@kalzoo kalzoo commented Jun 5, 2024

Make Instruction and all of its sub-structs both Hash and Eq:

  • Handle f64 equality as NaN == NaN which is defensible in the case of equating instructions
  • Handle IndexMap equality by relying on iteration order. This is suboptimal in that ordering shouldn't matter for equality checks, but sorting them would be expensive. This is where BTreeMap may actually make more sense, but that doesn't need to be addressed here. This was discussed in the past and I recall that there was a good reason not to select it, even if I forget that reason now.

TODO

  • right now, we just impl Eq for TypeX when it contains f64, which in effect will just cause a panic when one of those f64 is NaN. this should be better handled and possibly include a PartialEq override.

@github-actions
Copy link

github-actions bot commented Jun 5, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-374/
on branch quil-py-docs at 2024-06-05 23:59 UTC

@antalsz
Copy link
Contributor

antalsz commented Nov 17, 2025

@asaites Am I correct that this has been superseded by the latest work and is closable?

@asaites
Copy link
Contributor

asaites commented Nov 19, 2025

@antalsz The latest version of quil-rs has better float handling (notably, NaN == NaN), but not all Instructions support Eq and Hash. As is done in this PR (ignoring some upstream name changes), the following changes are needed:

Type +Eq +Hash +frozen +hash -set_all
Arithmetic +
Comparison +
Exchange +
Move +
Store +
Instruction +
Capture + + + -
CircuitDefinition + + + + -
FrameDefinition Manual + + -
Jump + + + -
JumpUnless + + + -
JumpWhen + + + -
MeasureCalibrationDefinition + + + + -
Pulse + + + -
WaveformInvocation + Manual + +
MeasureCalibrationIdentifier + Manual + + -

Adding Eq is of relatively little consequence. I separated those that add Hash, because for their Python counterparts, adding frozen and hash/removing set_all would be breaking changes for quil and subsequently PyQuil.

Those final two aren't actually Instructions, but they're used within fields of Instructions, and so would need support. The ones marked Manual involve IndexMaps that would need manual Hash implementations.

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