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.

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

Remove sort_key attribute from DAGNode #13736

Merged
merged 2 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions crates/accelerate/src/gate_direction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use hashbrown::HashSet;
use pyo3::intern;
use pyo3::prelude::*;
use pyo3::types::PyTuple;
use pyo3::IntoPyObjectExt;
use qiskit_circuit::operations::OperationRef;
use qiskit_circuit::packed_instruction::PackedOperation;
use qiskit_circuit::{
Expand Down Expand Up @@ -399,7 +398,6 @@ fn has_calibration_for_op_node(
#[cfg(feature = "cache_pygates")]
py_op: packed_inst.py_op.clone(),
},
sort_key: "".into_py_any(py)?,
},
DAGNode { node: None },
),
Expand Down
23 changes: 10 additions & 13 deletions crates/circuit/src/dag_circuit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2674,9 +2674,8 @@ def _format(operand):
///
/// Args:
/// key (Callable): A callable which will take a DAGNode object and
/// return a string sort key. If not specified the
/// :attr:`~qiskit.dagcircuit.DAGNode.sort_key` attribute will be
/// used as the sort key for each node.
/// return a string sort key. If not specified the bit qargs and
/// cargs of a node will be used for sorting.
///
/// Returns:
/// generator(DAGOpNode, DAGInNode, or DAGOutNode): node in topological order
Expand Down Expand Up @@ -2710,9 +2709,8 @@ def _format(operand):
///
/// Args:
/// key (Callable): A callable which will take a DAGNode object and
/// return a string sort key. If not specified the
/// :attr:`~qiskit.dagcircuit.DAGNode.sort_key` attribute will be
/// used as the sort key for each node.
/// return a string sort key. If not specified the qargs and
/// cargs of a node will be used for sorting.
///
/// Returns:
/// generator(DAGOpNode): op node in topological order
Expand Down Expand Up @@ -5612,22 +5610,22 @@ impl DAGCircuit {
let dag_node = match weight {
NodeType::QubitIn(qubit) => Py::new(
py,
DAGInNode::new(py, id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
DAGInNode::new(id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
)?
.into_any(),
NodeType::QubitOut(qubit) => Py::new(
py,
DAGOutNode::new(py, id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
DAGOutNode::new(id, self.qubits.get(*qubit).unwrap().clone_ref(py)),
)?
.into_any(),
NodeType::ClbitIn(clbit) => Py::new(
py,
DAGInNode::new(py, id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
DAGInNode::new(id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
)?
.into_any(),
NodeType::ClbitOut(clbit) => Py::new(
py,
DAGOutNode::new(py, id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
DAGOutNode::new(id, self.clbits.get(*clbit).unwrap().clone_ref(py)),
)?
.into_any(),
NodeType::Operation(packed) => {
Expand All @@ -5646,7 +5644,6 @@ impl DAGCircuit {
#[cfg(feature = "cache_pygates")]
py_op: packed.py_op.clone(),
},
sort_key: format!("{:?}", self.sort_key(id)).into_py_any(py)?,
},
DAGNode { node: Some(id) },
),
Expand All @@ -5655,12 +5652,12 @@ impl DAGCircuit {
}
NodeType::VarIn(var) => Py::new(
py,
DAGInNode::new(py, id, self.vars.get(*var).unwrap().clone_ref(py)),
DAGInNode::new(id, self.vars.get(*var).unwrap().clone_ref(py)),
)?
.into_any(),
NodeType::VarOut(var) => Py::new(
py,
DAGOutNode::new(py, id, self.vars.get(*var).unwrap().clone_ref(py)),
DAGOutNode::new(id, self.vars.get(*var).unwrap().clone_ref(py)),
)?
.into_any(),
};
Expand Down
79 changes: 16 additions & 63 deletions crates/circuit/src/dag_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ impl DAGNode {
#[pyclass(module = "qiskit._accelerate.circuit", extends=DAGNode)]
pub struct DAGOpNode {
pub instruction: CircuitInstruction,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Maybe this should become a tuple struct now. Feel free to leave as is if that's your preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept it this way for simplicity since I just deleted the lines with the previous attribute. I did overlook there is only a single field now. But I think I'd probably rather keep it the same because I feel like there are a few places we do node.instruction/self.instruction and that feels a bit more ergonomic than node.0

#[pyo3(get)]
pub sort_key: PyObject,
}

#[pymethods]
Expand All @@ -131,7 +129,6 @@ impl DAGOpNode {
) -> PyResult<Py<Self>> {
let py_op = op.extract::<OperationFromPython>()?;
let qargs = qargs.map_or_else(|| PyTuple::empty(py), |q| q.value);
let sort_key = qargs.str().unwrap().into();
let cargs = cargs.map_or_else(|| PyTuple::empty(py), |c| c.value);
let instruction = CircuitInstruction {
operation: py_op.operation,
Expand All @@ -143,16 +140,7 @@ impl DAGOpNode {
py_op: op.unbind().into(),
};

Py::new(
py,
(
DAGOpNode {
instruction,
sort_key,
},
DAGNode { node: None },
),
)
Py::new(py, (DAGOpNode { instruction }, DAGNode { node: None }))
}

fn __hash__(slf: PyRef<'_, Self>) -> PyResult<u64> {
Expand Down Expand Up @@ -239,7 +227,6 @@ impl DAGOpNode {
mut instruction: CircuitInstruction,
deepcopy: bool,
) -> PyResult<PyObject> {
let sort_key = instruction.qubits.bind(py).str().unwrap().into();
if deepcopy {
instruction.operation = instruction.operation.py_deepcopy(py, None)?;
#[cfg(feature = "cache_pygates")]
Expand All @@ -248,15 +235,12 @@ impl DAGOpNode {
}
}
let base = PyClassInitializer::from(DAGNode { node: None });
let sub = base.add_subclass(DAGOpNode {
instruction,
sort_key,
});
let sub = base.add_subclass(DAGOpNode { instruction });
Py::new(py, sub)?.into_py_any(py)
}

fn __reduce__(slf: PyRef<Self>, py: Python) -> PyResult<PyObject> {
let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
let state = slf.as_ref().node.map(|node| node.index());
let temp = (
slf.instruction.get_operation(py)?,
&slf.instruction.qubits,
Expand All @@ -266,9 +250,8 @@ impl DAGOpNode {
}

fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
let index: Option<usize> = state.extract()?;
slf.as_mut().node = index.map(NodeIndex::new);
slf.sort_key = sort_key;
Ok(())
}

Expand Down Expand Up @@ -461,44 +444,29 @@ impl DAGOpNode {
pub struct DAGInNode {
#[pyo3(get)]
pub wire: PyObject,
#[pyo3(get)]
sort_key: PyObject,
}

impl DAGInNode {
pub fn new(py: Python, node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
(
DAGInNode {
wire,
sort_key: intern!(py, "[]").clone().into(),
},
DAGNode { node: Some(node) },
)
pub fn new(node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
(DAGInNode { wire }, DAGNode { node: Some(node) })
}
}

#[pymethods]
impl DAGInNode {
#[new]
fn py_new(py: Python, wire: PyObject) -> PyResult<(Self, DAGNode)> {
Ok((
DAGInNode {
wire,
sort_key: intern!(py, "[]").clone().into(),
},
DAGNode { node: None },
))
fn py_new(wire: PyObject) -> PyResult<(Self, DAGNode)> {
Ok((DAGInNode { wire }, DAGNode { node: None }))
}

fn __reduce__<'py>(slf: PyRef<'py, Self>, py: Python<'py>) -> PyResult<Bound<'py, PyTuple>> {
let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
let state = slf.as_ref().node.map(|node| node.index());
(py.get_type::<Self>(), (&slf.wire,), state).into_pyobject(py)
}

fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
let index: Option<usize> = state.extract()?;
slf.as_mut().node = index.map(NodeIndex::new);
slf.sort_key = sort_key;
Ok(())
}

Expand Down Expand Up @@ -534,44 +502,29 @@ impl DAGInNode {
pub struct DAGOutNode {
#[pyo3(get)]
pub wire: PyObject,
#[pyo3(get)]
sort_key: PyObject,
}

impl DAGOutNode {
pub fn new(py: Python, node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
(
DAGOutNode {
wire,
sort_key: intern!(py, "[]").clone().into(),
},
DAGNode { node: Some(node) },
)
pub fn new(node: NodeIndex, wire: PyObject) -> (Self, DAGNode) {
(DAGOutNode { wire }, DAGNode { node: Some(node) })
}
}

#[pymethods]
impl DAGOutNode {
#[new]
fn py_new(py: Python, wire: PyObject) -> PyResult<(Self, DAGNode)> {
Ok((
DAGOutNode {
wire,
sort_key: intern!(py, "[]").clone().into(),
},
DAGNode { node: None },
))
fn py_new(wire: PyObject) -> PyResult<(Self, DAGNode)> {
Ok((DAGOutNode { wire }, DAGNode { node: None }))
}

fn __reduce__(slf: PyRef<Self>, py: Python) -> PyResult<PyObject> {
let state = (slf.as_ref().node.map(|node| node.index()), &slf.sort_key);
let state = slf.as_ref().node.map(|node| node.index());
(py.get_type::<Self>(), (&slf.wire,), state).into_py_any(py)
}

fn __setstate__(mut slf: PyRefMut<Self>, state: &Bound<PyAny>) -> PyResult<()> {
let (index, sort_key): (Option<usize>, PyObject) = state.extract()?;
let index: Option<usize> = state.extract()?;
slf.as_mut().node = index.map(NodeIndex::new);
slf.sort_key = sort_key;
Ok(())
}

Expand Down
5 changes: 4 additions & 1 deletion qiskit/dagcircuit/dagdependency_v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"""_DAGDependencyV2 class for representing non-commutativity in a circuit.
"""

import itertools
import math
from collections import OrderedDict, defaultdict, namedtuple
from typing import Dict, List, Generator, Any
Expand Down Expand Up @@ -459,7 +460,9 @@ def topological_nodes(self, key=None) -> Generator[DAGOpNode, Any, Any]:
"""

def _key(x):
return x.sort_key
return ",".join(
f"{self.find_bit(q).index:04d}" for q in itertools.chain(x.qargs, x.cargs)
)

if key is None:
key = _key
Expand Down
19 changes: 17 additions & 2 deletions qiskit/transpiler/passes/routing/star_prerouting.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,20 @@
# that they have been altered from the originals.

"""Search for star connectivity patterns and replace them with."""
import itertools
from typing import Iterable, Union, Optional, List, Tuple
from math import floor, log10

from qiskit.circuit import SwitchCaseOp, Clbit, ClassicalRegister, Barrier
from qiskit.circuit.controlflow import condition_resources, node_resources
from qiskit.dagcircuit import DAGOpNode, DAGDepNode, DAGDependency, DAGCircuit
from qiskit.dagcircuit import (
DAGOpNode,
DAGDepNode,
DAGDependency,
DAGCircuit,
DAGOutNode,
DAGInNode,
)
from qiskit.transpiler.basepasses import TransformationPass
from qiskit.transpiler.layout import Layout
from qiskit.transpiler.passes.routing.sabre_swap import _build_sabre_dag, _apply_sabre_result
Expand Down Expand Up @@ -331,7 +339,14 @@ def star_preroute(self, dag, blocks, processing_order):
}

def tie_breaker_key(node):
return processing_order_index_map.get(node, node.sort_key)
processing_order = processing_order_index_map.get(node, None)
if processing_order is not None:
return processing_order
if isinstance(node, (DAGInNode, DAGOutNode)):
return str(node.wire)
return ",".join(
f"{dag.find_bit(q).index:04d}" for q in itertools.chain(node.qargs, node.cargs)
)

rust_processing_order = _extract_nodes(dag.topological_op_nodes(key=tie_breaker_key), dag)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
upgrade_transpiler:
- |
Removed the deprecated ``DAGNode.sort_key`` attribute. This attribute was deprecated
in the Qiskit 1.4.0 release. As the lexicographical topological sorting is done internally
and rust and the sort key attribute was unused this was removed to remove the overhead
from DAG node creation. If you were relying on the sort key you can reproduce it from
a given node using something like::

def get_sort_key(node: DAGNode):
if isinstance(node, (DAGInNode, DAGOutNode)):
return str(node.wire)
return ",".join(
f"{dag.find_bit(q).index:04d}" for q in itertools.chain(node.qargs, node.cargs)
)
Loading