search

rigetti / quil-rs

Quil Parser & Program Builder
https://rigetti.github.io/quil-rs/
Apache License 2.0
20 stars 9 forks source link

feat!: Program instruction iteration and serialization is deterministic. #355

Closed MarquessV closed 6 months ago

MarquessV commented 6 months ago

Closes #353

github-actions[bot] commented 6 months ago

PR Preview Action v1.4.7 :---: :rocket: Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-355/ on branch quil-py-docs at 2024-04-16 22:45 UTC

kalzoo commented 6 months ago

Using index_map would also help solve the problem of duplicate calibrations.

MarquessV commented 6 months ago

Opened rigetti/rigetti-pyo3#43 to add support for IndexMap.

MarquessV commented 6 months ago

@Shadow53 @kalzoo - Is there a good reason not to replace all instances of BTreeMap/HashMap in the public API with IndexMap? Theoretically, if offers better insert performance, and it arguably provides better UX since users can expect instructions to be ordered the same way they inserted them (with respect to how Program groups them, of course).

EDIT: Updated this PR w/ those changes.

kalzoo commented 6 months ago

@Shadow53 @kalzoo - Is there a good reason not to replace all instances of BTreeMap/HashMap in the public API with IndexMap? Theoretically, if offers better insert performance, and it arguably provides better UX since users can expect instructions to be ordered the same way they inserted them (with respect to how Program groups them, of course).

EDIT: Updated this PR w/ those changes.

Our criteria should be:

  1. equality is correct (e.g. ordering does not matter for frames, does matter for defcals)
  2. duplication behavior is correct (i.e. all of them act as a set and duplicates are not stored)
  3. best performance as used; I'd hazard a guess that iteration speed is the most important, followed by insertion and then cloning.

So if indexmap checks those boxes, I'm good with it.