-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Port the PadDynamicalDecoupling pass from Python to Rust. #14735
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
base: main
Are you sure you want to change the base?
Port the PadDynamicalDecoupling pass from Python to Rust. #14735
Conversation
…to rust. The _pad method is the core logic that is called during the pass, whereas the run method would need to be ported along with the PadDelay pass, since it is defined in the BasePadding class.
crates/circuit/src/bit.rs
Outdated
pub fn index(&self) -> usize { | ||
match &self.0 { | ||
BitInfo::Owned { index, .. } => *index as usize, | ||
BitInfo::Anonymous { .. } => panic!("Anonymous qubit has no index"), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this panic may be a bit extreme, remember that a panic is an unrecoverable error. Perhaps having this method return an Option<usize>
might be better since the user would be able to easily handle a case where an index doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we really need this index
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinhartman I just thought that it would be useful to have in general, and would make the code easier to understand if the internal implementation details of the Owned
and Anonymous
variants of BitInfo
are abstracted away in passes like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far.
Just adding a couple of comments from our discussion earlier this week so we can keep track of them.
} else if extra_slack_distribution == "edges" { | ||
let to_begin_edge: f64 = constrained_length_scalar(alignment as f64, extra_slack / 2.0); | ||
taus[0] += to_begin_edge; | ||
taus[taus_len - 1] = extra_slack - to_begin_edge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taus[taus_len - 1] = extra_slack - to_begin_edge; | |
taus[taus_len - 1] += extra_slack - to_begin_edge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I'll apply the suggestion 👍
|
||
let node_start_time_obj = property_set.get_item("node_start_time")?; | ||
let node_start_time_dict = node_start_time_obj.downcast::<PyDict>()?; | ||
let start_time_opt = node_start_time_dict.del_item(prev_node).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think del_item
does not actually return anything, so start_time_opt
might actually be ()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, that's right. I should get it first, then delete it afterwards 👍.
next_node_inst.params = smallvec![ | ||
Param::Float(theta_new), | ||
Param::Float(phi_new), | ||
Param::Float(lam_new), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely won't work, since a CircuitInstruction
on a DAGNode
would be ephemeral (I think). You might need to replace the node in the DAG instead for this to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for pointing this out. I'll go ahead and re-implement it to replace the node instead 👍.
![]() |
Summary
_pad
method within thePadDynamicalDecoupling
pass from Python to Rust.PadDynamicalDecoupling
to Rust #14620.Details and comments
Updates the
_pad
method (inqiskit/transpiler/passes/scheduling/padding/dynamical_decoupling.py
) to call a rust routine instead.The reason that the
_pad
method is updated is that it is the core logic that runs when the pass is called, and therun
method inBasePadding
(defined inqiskit/transpiler/passes/scheduling/padding/base_padding.py
) calls it for bothPadDynamicalDecoupling
as well asPadDelay
. Hence, it would make more sense to fully port therun
method once the_pad
ofPadDelay
is also implemented in rust (as per PortPadDelay
to Rust #12266).Note: This is currently opened as a draft PR since the current implementation fails the test cases. I would appreciate it if any suggestions/comments can be provided as to what may be wrong here. Thank you.