Oxidize VariableMapper and uses.
#14361
Conversation
...and add very unpleasant error handling code.
kevinhartman
force-pushed
the
ox-var-mapper
branch
from
cf2fe19 to
a420113
Compare
|
One or more of the following people are relevant to this code:
|
mtreinish
added
performance
Changelog: None
Pull Request Test Coverage Report for Build 15690052436Details
💛 - Coveralls |
There was a problem hiding this comment.
Overall this looks really good thanks for doing that. I have a couple inline comments but nothing major.
The other question I have is there a reason to keep the python space mapper class around anymore? I'm guessing the answer is QuantumCircuit.compose() as that's the only usage I can find of it. That's probably blocked on #14299 so we might want to open a tracking issue for that.
| fn may_have_additional_wires(&self, op: OperationRef) -> bool { | ||
| let OperationRef::Instruction(inst) = op else { |
There was a problem hiding this comment.
Not that this is a big deal, but was there a reason for this change. It's probably better like this, but I'm just wondering if there was a motivation on this?
There was a problem hiding this comment.
I don't actually recall why I'd done this originally. FWIW, I believe this changes again in the control flow oxidation PR.
|
|
||
| pub(crate) struct VariableMapper { | ||
| target_cregs: Vec<ClassicalRegister>, | ||
| register_map: RefCell<HashMap<String, ClassicalRegister>>, |
There was a problem hiding this comment.
I'm curious why does this need interior mutability? Are we returning references to registers and need to mutate it as well?
There was a problem hiding this comment.
The reason is just that this is functionally a cache, only. Without the interior mutability, I'd need to make all of these map functions take a &mut self, and that had less than ideal implications when working with this struct. IIRC, caching is the official Rust docs example for interior mutability, so I feel good about the use of it here lol
Thanks for the initial review! Yeah, you're spot on—we need to port |
| let qargs_list: Vec<&ShareableQubit> = self | ||
| .qubits | ||
| .map_indices(self.qargs_interner.get(node.qubits)) | ||
| .collect(); | ||
| let mut cargs_list: Vec<&ShareableClbit> = self | ||
| .clbits | ||
| .map_indices(self.cargs_interner.get(node.clbits)) | ||
| .collect(); | ||
| let cargs_set: HashSet<&ShareableClbit> = | ||
| HashSet::from_iter(cargs_list.iter().cloned()); |
| } | ||
| // This is maintaining the legacy behavior of `DAGCircuit.compose`. We don't | ||
| // attempt to speed-up this lookup with a cache, since that would just make the more | ||
| // standard cases more annoying to deal with. |
There was a problem hiding this comment.
| } | |
| // This is maintaining the legacy behavior of `DAGCircuit.compose`. We don't | |
| // attempt to speed-up this lookup with a cache, since that would just make the more | |
| // standard cases more annoying to deal with. | |
| } | |
| // This is maintaining the legacy behavior of `DAGCircuit.compose`. We don't | |
| // attempt to speed-up this lookup with a cache, since that would just make the | |
| // standard cases more annoying to deal with. |
Saying "more" twice here seems a bit redundant.
There was a problem hiding this comment.
I will defer this since the comment is just ported from the Python version 🙂
Summary
Ports the private Python-space helper class
VariableMapperto Rust.Also introduces Rust-side iteration methods for classical expressions (
Expr).Details and comments
Note that we still will need the
Pythontoken inDAGCircuit::{py_substitute_node_with_dag, compose}until we (I) finish porting control flow.To-do
DAGCircuit::composeto use newVariableMapper.Make the existing Python-space variable mapper use the new one internally (we still need the Python interface forEdit: I'm going to defer this to whoever portsQuantumCircuit.QuantumCircuit.composeto Rust, rather than introducing some kind ofPyVariableMapperwrapper that would go away during that process anyhow.Stretchis being mapped properly byDAGCircuit::py_substitute_node_with_dagEdit: onlytargetandconditionget mapped, and they'd never containStretchso this is all set.