From cb7876bf89ee231c2aefb2f04da14ffce1042c36 Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Mon, 8 Jul 2024 16:30:50 -0400 Subject: [PATCH 1/6] Initial: Implement a nullable dict-like structure for IndexMap --- .../accelerate/src/target_transpiler/mod.rs | 95 ++--- .../target_transpiler/nullable_index_map.rs | 360 ++++++++++++++++++ 2 files changed, 411 insertions(+), 44 deletions(-) create mode 100644 crates/accelerate/src/target_transpiler/nullable_index_map.rs diff --git a/crates/accelerate/src/target_transpiler/mod.rs b/crates/accelerate/src/target_transpiler/mod.rs index 4f6b14f09c77..22fcef5fa907 100644 --- a/crates/accelerate/src/target_transpiler/mod.rs +++ b/crates/accelerate/src/target_transpiler/mod.rs @@ -14,6 +14,7 @@ mod errors; mod instruction_properties; +mod nullable_index_map; use std::ops::Index; @@ -22,6 +23,7 @@ use ahash::RandomState; use ahash::HashSet; use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; +use nullable_index_map::NullableIndexMap; use pyo3::{ exceptions::{PyAttributeError, PyIndexError, PyKeyError, PyValueError}, prelude::*, @@ -31,7 +33,6 @@ use pyo3::{ use qiskit_circuit::circuit_instruction::convert_py_to_operation_type; use qiskit_circuit::operations::{Operation, OperationType, Param}; -use rustworkx_core::dictmap::InitWithHasher; use smallvec::SmallVec; use crate::nlayout::PhysicalQubit; @@ -50,11 +51,11 @@ mod exceptions { // Custom types type Qargs = SmallVec<[PhysicalQubit; 2]>; type GateMap = IndexMap; -type PropsMap = IndexMap, Option, RandomState>; +type PropsMap = NullableIndexMap>; type GateMapState = Vec<(String, Vec<(Option, Option)>)>; #[derive(Debug, Clone, FromPyObject)] -enum TargetOperation { +pub enum TargetOperation { Normal(NormalOperation), Variadic(PyObject), } @@ -94,7 +95,7 @@ impl TargetOperation { } #[derive(Debug, Clone)] -struct NormalOperation { +pub struct NormalOperation { operation: OperationType, params: SmallVec<[Param; 3]>, op_object: PyObject, @@ -167,7 +168,7 @@ pub struct Target { _gate_name_map: IndexMap, global_operations: IndexMap, RandomState>, variable_class_operations: IndexSet, - qarg_gate_map: IndexMap, Option>, RandomState>, + qarg_gate_map: NullableIndexMap>>, non_global_strict_basis: Option>, non_global_basis: Option>, } @@ -264,7 +265,7 @@ impl Target { _gate_name_map: IndexMap::default(), variable_class_operations: IndexSet::default(), global_operations: IndexMap::default(), - qarg_gate_map: IndexMap::default(), + qarg_gate_map: NullableIndexMap::default(), non_global_basis: None, non_global_strict_basis: None, }) @@ -359,7 +360,7 @@ impl Target { if let Some(properties) = properties.as_mut() { qargs_val = PropsMap::with_capacity(properties.len()); let inst_num_qubits = instruction.num_qubits(); - if properties.contains_key(&None) { + if properties.contains_key(None) { self.global_operations .entry(inst_num_qubits) .and_modify(|e| { @@ -367,7 +368,8 @@ impl Target { }) .or_insert(HashSet::from_iter([name.to_string()])); } - let property_keys: Vec> = properties.keys().cloned().collect(); + let property_keys: Vec> = + properties.keys().map(|qargs| qargs.cloned()).collect(); for qarg in property_keys { if let Some(qarg) = qarg.as_ref() { if qarg.len() != inst_num_qubits as usize { @@ -388,16 +390,24 @@ impl Target { }) + 1, )); } - let inst_properties = properties.swap_remove(&qarg).unwrap(); + let inst_properties = properties.swap_remove(qarg.as_ref()).unwrap(); qargs_val.insert(qarg.clone(), inst_properties); - self.qarg_gate_map - .entry(qarg) - .and_modify(|e| { - if let Some(e) = e { - e.insert(name.to_string()); - } - }) - .or_insert(Some(HashSet::from_iter([name.to_string()]))); + if let Some(value) = self.qarg_gate_map.get_mut(qarg.as_ref()) { + if let Some(value) = value { + value.insert(name.to_string()); + } + } else { + self.qarg_gate_map + .insert(qarg, Some(HashSet::from_iter([name.to_string()]))); + } + // self.qarg_gate_map + // .entry(qarg) + // .and_modify(|e| { + // if let Some(e) = e { + // e.insert(name.to_string()); + // } + // }) + // .or_insert(Some(HashSet::from_iter([name.to_string()]))); } } else { qargs_val = PropsMap::with_capacity(0); @@ -433,14 +443,19 @@ impl Target { ))); }; let mut prop_map = self[&instruction].clone(); - if !(prop_map.contains_key(&qargs)) { + if !(prop_map.contains_key(qargs.as_ref())) { return Err(PyKeyError::new_err(format!( "Provided qarg {:?} not in this Target for {:?}.", &qargs.unwrap_or_default(), &instruction ))); } - prop_map.entry(qargs).and_modify(|e| *e = properties); + prop_map + .get_mut(qargs.as_ref()) + .map(|e| { + *e = properties; + + }); self.gate_map .entry(instruction) .and_modify(|e| *e = prop_map); @@ -522,7 +537,7 @@ impl Target { /// KeyError: If ``qargs`` is not in target #[pyo3(name = "operation_names_for_qargs", signature=(qargs=None, /))] pub fn py_operation_names_for_qargs(&self, qargs: Option) -> PyResult> { - match self.operation_names_for_qargs(&qargs) { + match self.operation_names_for_qargs(qargs.as_ref()) { Ok(set) => Ok(set), Err(e) => Err(PyKeyError::new_err(e.message)), } @@ -632,10 +647,10 @@ impl Target { if let Some(_qargs) = &qargs { if self.gate_map.contains_key(op_name) { let gate_map_name = &self.gate_map[op_name]; - if gate_map_name.contains_key(&qargs) { + if gate_map_name.contains_key(qargs.as_ref()) { return Ok(true); } - if gate_map_name.contains_key(&None) { + if gate_map_name.contains_key(None) { let qubit_comparison = self._gate_name_map[op_name].num_qubits(); return Ok(qubit_comparison == _qargs.len() as u32 @@ -692,7 +707,7 @@ impl Target { return Ok(true); } } - Ok(self.instruction_supported(&operation_name, &qargs)) + Ok(self.instruction_supported(&operation_name, qargs.as_ref())) } else { Ok(false) } @@ -914,7 +929,7 @@ impl Target { .unwrap() .extract::()? .into_iter() - .map(|(name, prop_map)| (name, IndexMap::from_iter(prop_map.into_iter()))), + .map(|(name, prop_map)| (name, PropsMap::from_iter(prop_map.into_iter()))), ); self._gate_name_map = state .get_item("gate_name_map")? @@ -924,7 +939,7 @@ impl Target { .get_item("global_operations")? .unwrap() .extract::, RandomState>>()?; - self.qarg_gate_map = IndexMap::from_iter( + self.qarg_gate_map = NullableIndexMap::from_iter( state .get_item("qarg_gate_map")? .unwrap() @@ -962,7 +977,7 @@ impl Target { self.gate_map.iter().flat_map(move |(op, props_map)| { props_map .keys() - .map(move |qargs| (&self._gate_name_map[op], qargs.as_ref())) + .map(move |qargs| (&self._gate_name_map[op], qargs)) }) } /// Returns an iterator over the operation names in the target. @@ -971,15 +986,7 @@ impl Target { } /// Get the operation objects in the target. - pub fn operations(&self) -> impl Iterator)> { - return self._operations().filter_map(|operation| match operation { - TargetOperation::Normal(normal) => Some((&normal.operation, &normal.params)), - _ => None, - }); - } - - /// Get the operation objects in the target. - fn _operations(&self) -> impl Iterator { + pub fn operations(&self) -> impl Iterator { return self._gate_name_map.values(); } @@ -1063,13 +1070,13 @@ impl Target { /// Gets all the operation names that use these qargs. Rust native equivalent of ``BaseTarget.operation_names_for_qargs()`` pub fn operation_names_for_qargs( &self, - qargs: &Option, + qargs: Option<&Qargs>, ) -> Result, TargetKeyError> { // When num_qubits == 0 we return globally defined operators let mut res: HashSet<&str> = HashSet::default(); let mut qargs = qargs; if self.num_qubits.unwrap_or_default() == 0 || self.num_qubits.is_none() { - qargs = &None; + qargs = None; } if let Some(qargs) = qargs.as_ref() { if qargs @@ -1107,7 +1114,7 @@ impl Target { /// Returns an iterator rust-native operation instances and parameters present in the Target that affect the provided qargs. pub fn ops_from_qargs( &self, - qargs: &Option, + qargs: Option<&Qargs>, ) -> Result)>, TargetKeyError> { match self.operation_names_for_qargs(qargs) { Ok(operations) => { @@ -1132,7 +1139,7 @@ impl Target { operation: &str, ) -> Result>, TargetKeyError> { if let Some(gate_map_oper) = self.gate_map.get(operation) { - if gate_map_oper.contains_key(&None) { + if gate_map_oper.contains_key(None) { return Ok(None); } let qargs = gate_map_oper.keys().flatten(); @@ -1183,18 +1190,18 @@ impl Target { if qargs.len() == 1 && is_none { return None; } - Some(qargs.map(|qarg| qarg.as_ref())) + Some(qargs) } - pub fn instruction_supported(&self, operation_name: &str, qargs: &Option) -> bool { + pub fn instruction_supported(&self, operation_name: &str, qargs: Option<&Qargs>) -> bool { if self.gate_map.contains_key(operation_name) { - if let Some(_qargs) = qargs.as_ref() { - let qarg_set: HashSet = _qargs.iter().cloned().collect(); + if let Some(_qargs) = qargs { + let qarg_set: HashSet<&PhysicalQubit> = _qargs.iter().collect(); if let Some(gate_prop_name) = self.gate_map.get(operation_name) { if gate_prop_name.contains_key(qargs) { return true; } - if gate_prop_name.contains_key(&None) { + if gate_prop_name.contains_key(None) { let obj = &self._gate_name_map[operation_name]; if self.variable_class_operations.contains(operation_name) { return qargs.is_none() diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs new file mode 100644 index 000000000000..ccb46a45e592 --- /dev/null +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -0,0 +1,360 @@ +use ahash::RandomState; +use indexmap::{ + map::{IntoIter as BaseIntoIter, Iter as BaseIter, Keys as BaseKeys, Values as BaseValues}, + IndexMap, +}; +use pyo3::prelude::*; +use pyo3::types::PyDict; +use pyo3::IntoPy; +use rustworkx_core::dictmap::InitWithHasher; +use std::ops::Index; +use std::{hash::Hash, mem::swap}; + +type BaseMap = IndexMap; + +#[derive(Debug, Clone)] +pub struct NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + map: BaseMap, + null_val: Option, +} + +impl NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + pub fn get(&self, key: Option<&K>) -> Option<&V> { + match key { + Some(key) => self.map.get(key), + None => self.null_val.as_ref(), + } + } + + pub fn get_mut(&mut self, key: Option<&K>) -> Option<&mut V> { + match key { + Some(key) => self.map.get_mut(key), + None => self.null_val.as_mut(), + } + } + + pub fn insert(&mut self, key: Option, value: V) -> Option { + match key { + Some(key) => self.map.insert(key, value), + None => { + let mut old_val = Some(value); + swap(&mut old_val, &mut self.null_val); + old_val + } + } + } + + pub fn with_capacity(n: usize) -> Self { + Self { + map: BaseMap::with_capacity(n), + null_val: None, + } + } + + pub fn from_iter<'a, I>(iter: I) -> Self + where + I: IntoIterator, V)> + 'a, + { + let mut null_val = None; + let filtered = iter.into_iter().filter_map(|item| match item { + (Some(key), value) => Some((key, value)), + (None, value) => { + null_val = Some(value); + None + } + }); + Self { + map: IndexMap::from_iter(filtered), + null_val, + } + } + + pub fn contains_key(&self, key: Option<&K>) -> bool { + match key { + Some(key) => self.map.contains_key(key), + None => self.null_val.is_some(), + } + } + + pub fn extend<'a, I>(&mut self, iter: I) + where + I: IntoIterator, V)> + 'a, + { + let filtered = iter.into_iter().filter_map(|item| match item { + (Some(key), value) => Some((key, value)), + (None, value) => { + self.null_val = Some(value); + None + } + }); + self.map.extend(filtered) + } + + pub fn swap_remove(&mut self, key: Option<&K>) -> Option { + match key { + Some(key) => self.map.swap_remove(key), + None => { + let mut ret_val = None; + swap(&mut ret_val, &mut self.null_val); + ret_val + } + } + } + + pub fn iter(&self) -> Iter { + Iter { + map: self.map.iter(), + null_value: &self.null_val, + } + } + + pub fn keys(&self) -> Keys { + Keys { + map_keys: self.map.keys(), + null_value: self.null_val.is_some(), + } + } + + pub fn values(&self) -> Values { + Values { + map_values: self.map.values(), + null_value: &self.null_val, + } + } + + pub fn len(&self) -> usize { + self.map.len() + self.null_val.is_some() as usize + } +} + +impl IntoIterator for NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + type Item = (Option, V); + type IntoIter = IntoIter; + + fn into_iter(self) -> Self::IntoIter { + IntoIter { + map: self.map.into_iter(), + null_value: self.null_val, + } + } +} + +pub struct Iter<'a, K, V> { + map: BaseIter<'a, K, V>, + null_value: &'a Option, +} + +impl<'a, K, V> Iterator for Iter<'a, K, V> { + type Item = (Option<&'a K>, &'a V); + + fn next(&mut self) -> Option { + if let Some((key, val)) = self.map.next() { + Some((Some(key), val)) + } else if let Some(value) = self.null_value { + let value = value; + self.null_value = &None; + Some((None, value)) + } else { + None + } + } + + fn size_hint(&self) -> (usize, Option) { + ( + self.map.size_hint().0 + self.null_value.is_some() as usize, + self.map + .size_hint() + .1 + .map(|hint| hint + self.null_value.is_some() as usize), + ) + } +} + +impl<'a, K, V> ExactSizeIterator for Iter<'a, K, V> { + fn len(&self) -> usize { + self.map.len() + self.null_value.is_some() as usize + } +} + +pub struct IntoIter { + map: BaseIntoIter, + null_value: Option, +} + +impl Iterator for IntoIter { + type Item = (Option, V); + + fn next(&mut self) -> Option { + if let Some((key, val)) = self.map.next() { + Some((Some(key), val)) + } else if self.null_value.is_some() { + let mut value = None; + swap(&mut value, &mut self.null_value); + Some((None, value.unwrap())) + } else { + None + } + } + + fn size_hint(&self) -> (usize, Option) { + ( + self.map.size_hint().0 + self.null_value.is_some() as usize, + self.map + .size_hint() + .1 + .map(|hint| hint + self.null_value.is_some() as usize), + ) + } +} + +impl<'a, K, V> ExactSizeIterator for IntoIter { + fn len(&self) -> usize { + self.map.len() + self.null_value.is_some() as usize + } +} + +pub struct Keys<'a, K, V> { + map_keys: BaseKeys<'a, K, V>, + null_value: bool, +} + +impl<'a, K, V> Iterator for Keys<'a, K, V> { + type Item = Option<&'a K>; + + fn next(&mut self) -> Option { + if let Some(key) = self.map_keys.next() { + Some(Some(key)) + } else if self.null_value { + Some(None) + } else { + None + } + } + + fn size_hint(&self) -> (usize, Option) { + ( + self.map_keys.size_hint().0 + self.null_value as usize, + self.map_keys + .size_hint() + .1 + .map(|hint| hint + self.null_value as usize), + ) + } +} + +impl<'a, K, V> ExactSizeIterator for Keys<'a, K, V> { + fn len(&self) -> usize { + self.map_keys.len() + self.null_value as usize + } +} + +pub struct Values<'a, K, V> { + map_values: BaseValues<'a, K, V>, + null_value: &'a Option, +} + +impl<'a, K, V> Iterator for Values<'a, K, V> { + type Item = &'a V; + + fn next(&mut self) -> Option { + if let Some(value) = self.map_values.next() { + Some(value) + } else if let Some(value) = self.null_value { + Some(value) + } else { + None + } + } + + fn size_hint(&self) -> (usize, Option) { + ( + self.map_values.size_hint().0 + self.null_value.is_some() as usize, + self.map_values + .size_hint() + .1 + .map(|hint| hint + self.null_value.is_some() as usize), + ) + } +} + +impl<'a, K, V> ExactSizeIterator for Values<'a, K, V> { + fn len(&self) -> usize { + self.map_values.len() + self.null_value.is_some() as usize + } +} + +impl Index> for NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + type Output = V; + fn index(&self, index: Option<&K>) -> &Self::Output { + match index { + Some(k) => self.map.index(k), + None => match &self.null_val { + Some(val) => val, + None => panic!("Null value not set"), + }, + } + } +} + +impl Default for NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + fn default() -> Self { + Self { + map: IndexMap::default(), + null_val: None, + } + } +} + +impl<'py, K, V> FromPyObject<'py> for NullableIndexMap +where + K: IntoPy + FromPyObject<'py> + Eq + Hash, + V: IntoPy + FromPyObject<'py>, +{ + fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { + let as_dict: &Bound = ob.downcast()?; + let null_val: Option = match as_dict + .get_item(ob.py().None())? + .map(|item| item.extract::()) + { + Some(value) => Some(value?), + None => None, + }; + let map: IndexMap = as_dict.extract()?; + Ok(Self { map, null_val }) + } +} + +impl IntoPy for NullableIndexMap +where + K: IntoPy + Eq + Hash, + V: IntoPy, +{ + fn into_py(self, py: Python<'_>) -> PyObject { + let map_object = self.map.into_py(py); + let downcast_dict = map_object.downcast_bound::(py).unwrap(); + downcast_dict + .set_item(py.None(), self.null_val.into_py(py)) + .unwrap(); + downcast_dict.to_object(py) + } +} From f1b426691ad1282f740abf9ff173593033e46aa6 Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Tue, 9 Jul 2024 14:39:11 -0400 Subject: [PATCH 2/6] FIx: Erroneous item extraction from Python - Fix error that caused `None` values to be ignored from `None` keys. - Removed mutability from rust function argument in `add_instruction`. - Object is mutably referenced after option unwrapping. - Add missing header in `nullable_index_map.rs`. - Add Clone as a `K` and/or `V` constraint in some of the iterators. - Remove `IntoPy` constraint from `NullableIndexMap`. - Add `ToPyObject` trait to `NullableIndexMap`. --- .../accelerate/src/target_transpiler/mod.rs | 27 ++--- .../target_transpiler/nullable_index_map.rs | 105 +++++++++++++----- qiskit/transpiler/target.py | 3 +- 3 files changed, 84 insertions(+), 51 deletions(-) diff --git a/crates/accelerate/src/target_transpiler/mod.rs b/crates/accelerate/src/target_transpiler/mod.rs index 22fcef5fa907..a1c4823dc116 100644 --- a/crates/accelerate/src/target_transpiler/mod.rs +++ b/crates/accelerate/src/target_transpiler/mod.rs @@ -341,7 +341,7 @@ impl Target { &mut self, instruction: TargetOperation, name: &str, - mut properties: Option, + properties: Option, ) -> PyResult<()> { if self.gate_map.contains_key(name) { return Err(PyAttributeError::new_err(format!( @@ -357,7 +357,7 @@ impl Target { self.variable_class_operations.insert(name.to_string()); } TargetOperation::Normal(_) => { - if let Some(properties) = properties.as_mut() { + if let Some(mut properties) = properties { qargs_val = PropsMap::with_capacity(properties.len()); let inst_num_qubits = instruction.num_qubits(); if properties.contains_key(None) { @@ -392,22 +392,12 @@ impl Target { } let inst_properties = properties.swap_remove(qarg.as_ref()).unwrap(); qargs_val.insert(qarg.clone(), inst_properties); - if let Some(value) = self.qarg_gate_map.get_mut(qarg.as_ref()) { - if let Some(value) = value { - value.insert(name.to_string()); - } + if let Some(Some(value)) = self.qarg_gate_map.get_mut(qarg.as_ref()) { + value.insert(name.to_string()); } else { self.qarg_gate_map .insert(qarg, Some(HashSet::from_iter([name.to_string()]))); } - // self.qarg_gate_map - // .entry(qarg) - // .and_modify(|e| { - // if let Some(e) = e { - // e.insert(name.to_string()); - // } - // }) - // .or_insert(Some(HashSet::from_iter([name.to_string()]))); } } else { qargs_val = PropsMap::with_capacity(0); @@ -450,12 +440,9 @@ impl Target { &instruction ))); } - prop_map - .get_mut(qargs.as_ref()) - .map(|e| { - *e = properties; - - }); + if let Some(e) = prop_map.get_mut(qargs.as_ref()) { + *e = properties; + } self.gate_map .entry(instruction) .and_modify(|e| *e = prop_map); diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs index ccb46a45e592..5005ab586ef3 100644 --- a/crates/accelerate/src/target_transpiler/nullable_index_map.rs +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -1,3 +1,15 @@ +// This code is part of Qiskit. +// +// (C) Copyright IBM 2024 +// +// This code is licensed under the Apache License, Version 2.0. You may +// obtain a copy of this license in the LICENSE.txt file in the root directory +// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. +// +// Any modifications or derivative works of this code must retain this +// copyright notice, and modified files need to carry a notice indicating +// that they have been altered from the originals. + use ahash::RandomState; use indexmap::{ map::{IntoIter as BaseIntoIter, Iter as BaseIter, Keys as BaseKeys, Values as BaseValues}, @@ -15,8 +27,8 @@ type BaseMap = IndexMap; #[derive(Debug, Clone)] pub struct NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: Eq + Hash + Clone, + V: Clone, { map: BaseMap, null_val: Option, @@ -24,8 +36,8 @@ where impl NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: Eq + Hash + Clone, + V: Clone, { pub fn get(&self, key: Option<&K>) -> Option<&V> { match key { @@ -137,8 +149,8 @@ where impl IntoIterator for NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: Eq + Hash + Clone, + V: Clone, { type Item = (Option, V); type IntoIter = IntoIter; @@ -188,12 +200,18 @@ impl<'a, K, V> ExactSizeIterator for Iter<'a, K, V> { } } -pub struct IntoIter { +pub struct IntoIter +where + V: Clone, +{ map: BaseIntoIter, null_value: Option, } -impl Iterator for IntoIter { +impl Iterator for IntoIter +where + V: Clone, +{ type Item = (Option, V); fn next(&mut self) -> Option { @@ -219,7 +237,10 @@ impl Iterator for IntoIter { } } -impl<'a, K, V> ExactSizeIterator for IntoIter { +impl ExactSizeIterator for IntoIter +where + V: Clone, +{ fn len(&self) -> usize { self.map.len() + self.null_value.is_some() as usize } @@ -237,6 +258,7 @@ impl<'a, K, V> Iterator for Keys<'a, K, V> { if let Some(key) = self.map_keys.next() { Some(Some(key)) } else if self.null_value { + self.null_value = false; Some(None) } else { None @@ -271,8 +293,10 @@ impl<'a, K, V> Iterator for Values<'a, K, V> { fn next(&mut self) -> Option { if let Some(value) = self.map_values.next() { Some(value) - } else if let Some(value) = self.null_value { - Some(value) + } else if self.null_value.is_some() { + let return_value = self.null_value; + self.null_value = &None; + return_value.as_ref() } else { None } @@ -297,8 +321,8 @@ impl<'a, K, V> ExactSizeIterator for Values<'a, K, V> { impl Index> for NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: Eq + Hash + Clone, + V: Clone, { type Output = V; fn index(&self, index: Option<&K>) -> &Self::Output { @@ -314,8 +338,8 @@ where impl Default for NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: Eq + Hash + Clone, + V: Clone, { fn default() -> Self { Self { @@ -327,17 +351,17 @@ where impl<'py, K, V> FromPyObject<'py> for NullableIndexMap where - K: IntoPy + FromPyObject<'py> + Eq + Hash, - V: IntoPy + FromPyObject<'py>, + K: IntoPy + FromPyObject<'py> + Eq + Hash + Clone, + V: IntoPy + FromPyObject<'py> + Clone, { fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { let as_dict: &Bound = ob.downcast()?; - let null_val: Option = match as_dict - .get_item(ob.py().None())? - .map(|item| item.extract::()) - { - Some(value) => Some(value?), - None => None, + let null_val: Option = if as_dict.contains(ob.py().None())? { + let val = Some(as_dict.get_item(ob.py().None())?.unwrap().extract()?); + as_dict.del_item(ob.py().None())?; + val + } else { + None }; let map: IndexMap = as_dict.extract()?; Ok(Self { map, null_val }) @@ -346,15 +370,36 @@ where impl IntoPy for NullableIndexMap where - K: IntoPy + Eq + Hash, - V: IntoPy, + K: IntoPy + Eq + Hash + Clone, + V: IntoPy + Clone, { fn into_py(self, py: Python<'_>) -> PyObject { let map_object = self.map.into_py(py); - let downcast_dict = map_object.downcast_bound::(py).unwrap(); - downcast_dict - .set_item(py.None(), self.null_val.into_py(py)) - .unwrap(); - downcast_dict.to_object(py) + let bound_map_obj = map_object.bind(py); + let downcast_dict: &Bound = bound_map_obj.downcast().unwrap(); + if let Some(null_val) = self.null_val { + downcast_dict + .set_item(py.None(), null_val.into_py(py)) + .unwrap(); + } + map_object + } +} + +impl ToPyObject for NullableIndexMap +where + K: ToPyObject + Eq + Hash + Clone, + V: ToPyObject + Clone, +{ + fn to_object(&self, py: Python<'_>) -> PyObject { + let map_object = self.map.to_object(py); + let bound_map_obj = map_object.bind(py); + let downcast_dict: &Bound = bound_map_obj.downcast().unwrap(); + if let Some(null_val) = &self.null_val { + downcast_dict + .set_item(py.None(), null_val.to_object(py)) + .unwrap(); + } + map_object } } diff --git a/qiskit/transpiler/target.py b/qiskit/transpiler/target.py index ffc112c4826a..a7df59e298bc 100644 --- a/qiskit/transpiler/target.py +++ b/qiskit/transpiler/target.py @@ -19,6 +19,7 @@ from __future__ import annotations +import copy import itertools from typing import Optional, List, Any @@ -423,7 +424,7 @@ def add_instruction(self, instruction, properties=None, name=None): properties = {None: None} if instruction_name in self._gate_map: raise AttributeError(f"Instruction {instruction_name} is already in the target") - super().add_instruction(instruction, instruction_name, properties) + super().add_instruction(instruction, instruction_name, copy.copy(properties)) self._gate_map[instruction_name] = properties self._coupling_graph = None self._instruction_durations = None From 1b9b502c36e7c9c7d13a77f58c0bbe73162721f5 Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Tue, 9 Jul 2024 15:01:03 -0400 Subject: [PATCH 3/6] Fix: inplace modification of Python dict. - Perform `None` extraction from rust. - Revert changes to `Target.py` --- .../target_transpiler/nullable_index_map.rs | 25 +++++++++++-------- qiskit/transpiler/target.py | 3 +-- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs index 5005ab586ef3..7f5f84297572 100644 --- a/crates/accelerate/src/target_transpiler/nullable_index_map.rs +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -355,16 +355,21 @@ where V: IntoPy + FromPyObject<'py> + Clone, { fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { - let as_dict: &Bound = ob.downcast()?; - let null_val: Option = if as_dict.contains(ob.py().None())? { - let val = Some(as_dict.get_item(ob.py().None())?.unwrap().extract()?); - as_dict.del_item(ob.py().None())?; - val - } else { - None - }; - let map: IndexMap = as_dict.extract()?; - Ok(Self { map, null_val }) + let map: IndexMap, V, RandomState> = ob.extract()?; + let mut null_val: Option = None; + let filtered = map + .into_iter() + .filter_map(|(key, value)| match (key, value) { + (Some(key), value) => Some((key, value)), + (None, value) => { + null_val = Some(value); + None + } + }); + Ok(Self { + map: filtered.collect(), + null_val, + }) } } diff --git a/qiskit/transpiler/target.py b/qiskit/transpiler/target.py index a7df59e298bc..ffc112c4826a 100644 --- a/qiskit/transpiler/target.py +++ b/qiskit/transpiler/target.py @@ -19,7 +19,6 @@ from __future__ import annotations -import copy import itertools from typing import Optional, List, Any @@ -424,7 +423,7 @@ def add_instruction(self, instruction, properties=None, name=None): properties = {None: None} if instruction_name in self._gate_map: raise AttributeError(f"Instruction {instruction_name} is already in the target") - super().add_instruction(instruction, instruction_name, copy.copy(properties)) + super().add_instruction(instruction, instruction_name, properties) self._gate_map[instruction_name] = properties self._coupling_graph = None self._instruction_durations = None From ab0ba29ee30ed439f0c5da0d5fa401894b51643e Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Tue, 9 Jul 2024 15:46:50 -0400 Subject: [PATCH 4/6] Fix: Avoid double iteration by using filter_map. --- .../src/target_transpiler/nullable_index_map.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs index 7f5f84297572..ea354a77860a 100644 --- a/crates/accelerate/src/target_transpiler/nullable_index_map.rs +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -355,19 +355,20 @@ where V: IntoPy + FromPyObject<'py> + Clone, { fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { - let map: IndexMap, V, RandomState> = ob.extract()?; - let mut null_val: Option = None; - let filtered = map - .into_iter() - .filter_map(|(key, value)| match (key, value) { + let mut null_val = None; + let dict_downcast: &Bound = ob.downcast()?; + let iter = dict_downcast.iter().filter_map(|(key, value)| { + let (key, value): (Option, V) = (key.extract().unwrap(), value.extract().unwrap()); + match (key, value) { (Some(key), value) => Some((key, value)), (None, value) => { null_val = Some(value); None } - }); + } + }); Ok(Self { - map: filtered.collect(), + map: iter.collect(), null_val, }) } From bb329af45f0e12df0df5c9f1e0fe6831031581cb Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Tue, 9 Jul 2024 19:48:22 -0400 Subject: [PATCH 5/6] Docs: Add inline comments. --- .../target_transpiler/nullable_index_map.rs | 60 ++++++++++++++++--- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs index ea354a77860a..d77fb73f4aab 100644 --- a/crates/accelerate/src/target_transpiler/nullable_index_map.rs +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -24,6 +24,18 @@ use std::{hash::Hash, mem::swap}; type BaseMap = IndexMap; +/// +/// An `IndexMap`-like structure thet can be used when one of the keys can have a `None` value. +/// +/// This structure is essentially a wrapper around the `IndexMap` struct that allows the +/// storage of `Option` key values as `K`` and keep an extra slot reserved only for the +/// `None` instance. There are some upsides to this including: +/// +/// The ability to index using Option<&K> to index a specific key. +/// Store keys as non option wrapped to obtain references to K instead of reference to Option. +/// +/// **Warning:** This is an experimental feature and should be used with care as it does not +/// fully implement all the methods present in `IndexMap` due to API limitations. #[derive(Debug, Clone)] pub struct NullableIndexMap where @@ -39,6 +51,8 @@ where K: Eq + Hash + Clone, V: Clone, { + /// Returns a reference to the value stored at `key`, if it does not exist + /// `None` is returned instead. pub fn get(&self, key: Option<&K>) -> Option<&V> { match key { Some(key) => self.map.get(key), @@ -46,6 +60,8 @@ where } } + /// Returns a mutable reference to the value stored at `key`, if it does not + /// exist `None` is returned instead. pub fn get_mut(&mut self, key: Option<&K>) -> Option<&mut V> { match key { Some(key) => self.map.get_mut(key), @@ -53,6 +69,10 @@ where } } + /// Inserts a `value` in the slot alotted to `key`. + /// + /// If a previous value existed there previously it will be returned, otherwise + /// `None` will be returned. pub fn insert(&mut self, key: Option, value: V) -> Option { match key { Some(key) => self.map.insert(key, value), @@ -64,6 +84,11 @@ where } } + /// Creates an instance of `NullableIndexMap` with capacity to hold `n`+1 key-value + /// pairs. + /// + /// Notice that an extra space needs to be alotted to store the instance of `None` a + /// key. pub fn with_capacity(n: usize) -> Self { Self { map: BaseMap::with_capacity(n), @@ -71,6 +96,8 @@ where } } + /// Creates an instance of `NullableIndexMap` from an iterator over instances of + /// `(Option, V)`. pub fn from_iter<'a, I>(iter: I) -> Self where I: IntoIterator, V)> + 'a, @@ -89,6 +116,7 @@ where } } + /// Returns `true` if the map contains a slot indexed by `key`, otherwise `false`. pub fn contains_key(&self, key: Option<&K>) -> bool { match key { Some(key) => self.map.contains_key(key), @@ -96,6 +124,11 @@ where } } + /// Extends the key-value pairs in the map with the contents of an iterator over + /// `(Option, V)`. + /// + /// If an already existent key is provided, it will be replaced by the entry provided + /// in the iterator. pub fn extend<'a, I>(&mut self, iter: I) where I: IntoIterator, V)> + 'a, @@ -110,6 +143,10 @@ where self.map.extend(filtered) } + /// Removes the entry allotted to `key` from the map and returns it. The index of + /// this entry is then replaced by the entry located at the last index. + /// + /// `None` will be returned if the `key` is not present in the map. pub fn swap_remove(&mut self, key: Option<&K>) -> Option { match key { Some(key) => self.map.swap_remove(key), @@ -121,6 +158,7 @@ where } } + /// Returns an iterator over references of the key-value pairs of the map. pub fn iter(&self) -> Iter { Iter { map: self.map.iter(), @@ -128,6 +166,7 @@ where } } + /// Returns an iterator over references of the keys present in the map. pub fn keys(&self) -> Keys { Keys { map_keys: self.map.keys(), @@ -135,6 +174,7 @@ where } } + /// Returns an iterator over references of all the values present in the map. pub fn values(&self) -> Values { Values { map_values: self.map.values(), @@ -142,6 +182,7 @@ where } } + /// Returns the number of key-value pairs present in the map. pub fn len(&self) -> usize { self.map.len() + self.null_val.is_some() as usize } @@ -163,6 +204,7 @@ where } } +/// Iterator for the key-value pairs in `NullableIndexMap`. pub struct Iter<'a, K, V> { map: BaseIter<'a, K, V>, null_value: &'a Option, @@ -200,6 +242,7 @@ impl<'a, K, V> ExactSizeIterator for Iter<'a, K, V> { } } +/// Owned iterator over the key-value pairs in `NullableIndexMap`. pub struct IntoIter where V: Clone, @@ -246,6 +289,7 @@ where } } +/// Iterator over the keys of a `NullableIndexMap`. pub struct Keys<'a, K, V> { map_keys: BaseKeys<'a, K, V>, null_value: bool, @@ -282,6 +326,7 @@ impl<'a, K, V> ExactSizeIterator for Keys<'a, K, V> { } } +/// Iterator over the values of a `NullableIndexMap`. pub struct Values<'a, K, V> { map_values: BaseValues<'a, K, V>, null_value: &'a Option, @@ -355,20 +400,19 @@ where V: IntoPy + FromPyObject<'py> + Clone, { fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { - let mut null_val = None; - let dict_downcast: &Bound = ob.downcast()?; - let iter = dict_downcast.iter().filter_map(|(key, value)| { - let (key, value): (Option, V) = (key.extract().unwrap(), value.extract().unwrap()); - match (key, value) { + let map: IndexMap, V, RandomState> = ob.extract()?; + let mut null_val: Option = None; + let filtered = map + .into_iter() + .filter_map(|(key, value)| match (key, value) { (Some(key), value) => Some((key, value)), (None, value) => { null_val = Some(value); None } - } - }); + }); Ok(Self { - map: iter.collect(), + map: filtered.collect(), null_val, }) } From 35f6768b479a7d8a8cab289f688a3d62760b0bc0 Mon Sep 17 00:00:00 2001 From: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Date: Wed, 10 Jul 2024 10:42:45 -0400 Subject: [PATCH 6/6] Fix: More specific error message in `NullableIndexMap` --- crates/accelerate/src/target_transpiler/nullable_index_map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/accelerate/src/target_transpiler/nullable_index_map.rs b/crates/accelerate/src/target_transpiler/nullable_index_map.rs index d77fb73f4aab..35d747660857 100644 --- a/crates/accelerate/src/target_transpiler/nullable_index_map.rs +++ b/crates/accelerate/src/target_transpiler/nullable_index_map.rs @@ -375,7 +375,7 @@ where Some(k) => self.map.index(k), None => match &self.null_val { Some(val) => val, - None => panic!("Null value not set"), + None => panic!("The provided key is not present in map: None"), }, } }