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

Add Operation::blocks method. #13056

Merged
merged 3 commits into from
Sep 19, 2024
+51 −6
Merged

Add Operation::blocks method. #13056

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
622509e
Add PyInstruction::blocks method.
kevinhartman Aug 29, 2024
591b0cb
Expose blocks at Operation level.
kevinhartman Sep 4, 2024
2a0951f
Add comment to explain unwraps.
kevinhartman Sep 19, 2024
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
53 changes: 47 additions & 6 deletions crates/circuit/src/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use approx::relative_eq;
use std::f64::consts::PI;
use std::vec;

use crate::circuit_data::CircuitData;
use crate::circuit_instruction::ExtraInstructionAttributes;
Expand Down Expand Up @@ -145,6 +146,7 @@ pub trait Operation {
fn num_clbits(&self) -> u32;
fn num_params(&self) -> u32;
fn control_flow(&self) -> bool;
fn blocks(&self) -> Vec<CircuitData>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since blocks will only return a populated Vec when dealing with a PyInstruction, could we make this accept a py token regardless? It is very likely that the user here will have obtained the GIL whenever they want to play with an instance of PyInstruction, it's all within the name after all.

I saw your previous comments when discussing with Jake about keeping this consistent. However, in this case, this method is only supposed to work with one type of operation, one that specifically holds a PyObject and is tied quite closely with python, and even then it is not guaranteed to be an instance of ControlFlowOp. The function itself will also perform callbacks to python and obtaining the GIL manually might prove itself costly here (it would be nice to benchmark it beforehand to see if this holds true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the case that right now blocks comes from Python, but once we've ported control flow operations to Rust that goes away. I don't see a compelling reason to ask for a Python token at the Operation level for blocks but not for its other methods, so I'd rather be consistent to avoid implying to code readers that there's some logical difference between Operation::blocks and say, Operation::matrix.

In the case of performance, having the GIL already means that with_gil will return fastest (as opposed to not holding it). And, we don't even try to acquire it unless this is a control flow operation, so the penalty to calling with_gil within Operation::blocks would scale with respect to the number of control flow operations in the circuit, rather than the number of op nodes.

fn matrix(&self, params: &[Param]) -> Option<Array2<Complex64>>;
fn definition(&self, params: &[Param]) -> Option<CircuitData>;
fn standard_gate(&self) -> Option<StandardGate>;
Expand Down Expand Up @@ -210,6 +212,15 @@ impl<'a> Operation for OperationRef<'a> {
}
}
#[inline]
fn blocks(&self) -> Vec<CircuitData> {
match self {
OperationRef::Standard(standard) => standard.blocks(),
OperationRef::Gate(gate) => gate.blocks(),
OperationRef::Instruction(instruction) => instruction.blocks(),
OperationRef::Operation(operation) => operation.blocks(),
}
}
#[inline]
fn matrix(&self, params: &[Param]) -> Option<Array2<Complex64>> {
match self {
Self::Standard(standard) => standard.matrix(params),
Expand Down Expand Up @@ -506,20 +517,20 @@ impl Operation for StandardGate {
STANDARD_GATE_NUM_QUBITS[*self as usize]
}

fn num_params(&self) -> u32 {
STANDARD_GATE_NUM_PARAMS[*self as usize]
}

fn num_clbits(&self) -> u32 {
0
}

fn num_params(&self) -> u32 {
STANDARD_GATE_NUM_PARAMS[*self as usize]
}

fn control_flow(&self) -> bool {
false
}

fn directive(&self) -> bool {
false
fn blocks(&self) -> Vec<CircuitData> {
vec![]
}

fn matrix(&self, params: &[Param]) -> Option<Array2<Complex64>> {
Expand Down Expand Up @@ -2019,6 +2030,10 @@ impl Operation for StandardGate {
fn standard_gate(&self) -> Option<StandardGate> {
Some(*self)
}

fn directive(&self) -> bool {
false
}
}

const FLOAT_ZERO: Param = Param::Float(0.0);
Expand Down Expand Up @@ -2103,6 +2118,26 @@ impl Operation for PyInstruction {
fn control_flow(&self) -> bool {
self.control_flow
}
fn blocks(&self) -> Vec<CircuitData> {
if !self.control_flow {
return vec![];
}
Python::with_gil(|py| -> Vec<CircuitData> {
// We expect that if PyInstruction::control_flow is true then the operation WILL
// have a 'blocks' attribute which is a tuple of the Python QuantumCircuit.
let raw_blocks = self.instruction.getattr(py, "blocks").unwrap();
let blocks: &Bound<PyTuple> = raw_blocks.downcast_bound::<PyTuple>(py).unwrap();
blocks
.iter()
.map(|b| {
b.getattr(intern!(py, "_data"))
.unwrap()
.extract::<CircuitData>()
.unwrap()
})
.collect()
})
}
fn matrix(&self, _params: &[Param]) -> Option<Array2<Complex64>> {
None
}
Expand Down Expand Up @@ -2169,6 +2204,9 @@ impl Operation for PyGate {
fn control_flow(&self) -> bool {
false
}
fn blocks(&self) -> Vec<CircuitData> {
vec![]
}
fn matrix(&self, _params: &[Param]) -> Option<Array2<Complex64>> {
Python::with_gil(|py| -> Option<Array2<Complex64>> {
match self.gate.getattr(py, intern!(py, "to_matrix")) {
Expand Down Expand Up @@ -2248,6 +2286,9 @@ impl Operation for PyOperation {
fn control_flow(&self) -> bool {
false
}
fn blocks(&self) -> Vec<CircuitData> {
vec![]
}
fn matrix(&self, _params: &[Param]) -> Option<Array2<Complex64>> {
None
}
Expand Down
4 changes: 4 additions & 0 deletions crates/circuit/src/packed_instruction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,10 @@ impl Operation for PackedOperation {
self.view().control_flow()
}
#[inline]
fn blocks(&self) -> Vec<CircuitData> {
self.view().blocks()
}
#[inline]
fn matrix(&self, params: &[Param]) -> Option<Array2<Complex64>> {
self.view().matrix(params)
}
Expand Down
Loading