Skip to content

feat!: make InstructionHandler trait generic#501

Open
mingyoungjeng wants to merge 3 commits intomainfrom
500-generic-instruction-handler
Open

feat!: make InstructionHandler trait generic#501
mingyoungjeng wants to merge 3 commits intomainfrom
500-generic-instruction-handler

Conversation

@mingyoungjeng
Copy link
Copy Markdown
Collaborator

@mingyoungjeng mingyoungjeng commented Mar 19, 2026

Closes #500

@mingyoungjeng mingyoungjeng self-assigned this Mar 19, 2026
@mingyoungjeng mingyoungjeng changed the title feat!: make instruction handler trait generic feat!: make InstructionHandler trait generic Mar 19, 2026
Copy link
Copy Markdown

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🤙

💡 To request another review, post a new comment with "/windsurf-review".

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 19, 2026

PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://rigetti.github.io/quil-rs/pr-preview/pr-501/

Built to branch quil-py-docs at 2026-03-20 04:47 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@mingyoungjeng mingyoungjeng marked this pull request as draft March 19, 2026 19:42
@mingyoungjeng mingyoungjeng marked this pull request as ready for review March 20, 2026 04:42
Copy link
Copy Markdown

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 To request another review, post a new comment with "/windsurf-review".

Comment thread quil-rs/src/instruction/mod.rs
Comment thread quil-rs/src/program/scheduling/graph.rs
.ok()
});
let common_sample_rate = handler
.matching_frames(&program.frames, program.get_used_qubits(), instruction)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to pass more specific parameters to matching_frames is good, but calling program.get_used_qubits() on each invocation might be inefficient if this function is called frequently. Consider caching the result of get_used_qubits() if this is in a performance-critical path.

/// [Quil-T]: https://quil-lang.github.io/#12Annex-T--Pulse-Level-Control
fn matching_frames<'f>(
&self,
frames: &'f FrameSet,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes more sense to pass a FrameSet here, and also means an entire Program isn't needed to use this functionality.

In an ideal world, it would be possible to eliminate qubits_available, which only applies to Resets in the default handler.
Perhaps a better design would be to make DefaultHandler include qubits_available as a field, but that propagated a bit too much for my taste.

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.

Allow implementors of InstructionHandler to use custom instruction sets

1 participant