Type annotations for the baseobjs - #733
Conversation
ndsieki
left a comment
ndsieki
left a comment
There was a problem hiding this comment.
I've indicated where type annotations are missing and also where existing type annotations may need modified.
There was a problem hiding this comment.
It appears that the parameter name has been changed from elemgen_basis to basis; the docstring should be updated accordingly.
There was a problem hiding this comment.
Since an ElementaryErrorgenBasis is a specific basis, I am updating the parameter to have the elemgen_basis name. I do not see any calls to create an ErrorgenSpace(vec, basis=foo) so this is an allowed change.
| def __init__(self, input_array, indices_to_protect=None, protected_index_mask= None): | ||
| def __init__(self, input_array: _np.ndarray, | ||
| indices_to_protect: Optional[Union[int, Iterable[Union[list[int], tuple]]]]=None, | ||
| protected_index_mask: _np.ndarray= None): |
There was a problem hiding this comment.
Shouldn't the type annotation for protected_index_mask be Optional[_np.ndarray] if None is an acceptable value?
There was a problem hiding this comment.
Yes, you are correct. One does need to be careful though when checking for None when the object can be a numpy array.
def check(foo):
if foo:
print(True)
else:
print(False)
check(None) will print False but check(np.arange(12)) will through a ValueError about checking the truth value of an array. One needs to check directly against None with an is None or is not None condition. That is already included in this code but I am writing this down for future reference.
|
|
||
| @classmethod | ||
| def common_graph(cls, num_qubits=0, geometry="line", directed=True, qubit_labels=None, all_directions=False): | ||
| def common_graph(cls, num_qubits=0, geometry: Literal['line','ring','grid','torus']="line", directed=True, |
There was a problem hiding this comment.
Please consider adding type annotations for num_qubits, directed, and all_directions.
|
|
||
|
|
||
| def make_rpe_data_set(model_or_dataset, string_list_d, num_samples, sample_error='binomial', seed=None): | ||
| def make_rpe_data_set(model_or_dataset, string_list_d, |
There was a problem hiding this comment.
Please add a type annotation for model_or_dataset and string_list_d.
| def create_bootstrap_dataset(input_data_set, generation_method, input_model=None, | ||
| seed=None, outcome_labels=None, verbosity=1): | ||
| def create_bootstrap_dataset(input_data_set: _DataSet, generation_method: Literal["nonparametric", "parametric"], | ||
| input_model=None, seed: Optional[int]=None, outcome_labels:Optional[list[_Label]]=None, |
There was a problem hiding this comment.
Please add a type annotation for input_model.
| generation_method: Literal["nonparametric", "parametric"], | ||
| fiducial_prep: list[_Circuit], fiducial_measure: list[_Circuit], | ||
| germs: list[_Circuit], max_lengths: list[int], | ||
| input_model=None, target_model=None, start_seed: int=0, |
There was a problem hiding this comment.
Please add a type annotation for input_model and target_model.
| input_model=None, target_model=None, start_seed: int=0, | ||
| outcome_labels: Optional[list[_Label]]=None, | ||
| lsgst_lists: Optional[list[list[_Circuit]]]=None, | ||
| return_data=False, verbosity=2): |
There was a problem hiding this comment.
Please add a type annotation for verbosity. Please consider adding a type annotation for return_data.
|
|
||
| def __init__(self, input_array, indices_to_protect=None, protected_index_mask= None): | ||
| def __init__(self, input_array: _np.ndarray, | ||
| indices_to_protect: Optional[Union[int, Iterable[Union[list[int], tuple]]]]=None, |
There was a problem hiding this comment.
This type annotation appears incorrect. For instance, indices_to_protect=(0,1) is handled properly by the __init__ function, but is not covered by the type annotation. The type annotation also does not include slice, even though slice is allowed in line 75. Also, although you have list[int], there is no parameterization specified for tuple.
You could consider something akin to baseobjs/qubitgraph.py as follows (not sure if the suggestion is exactly right since it is based on an LLM):
_IndexSpec = Sequence[Union[int, slice]]
indices_to_protect: Optional[Union[int, _IndexSpec, Iterable[_IndexSpec]]] = None
ndsieki
left a comment
ndsieki
left a comment
There was a problem hiding this comment.
While reviewing #720 I realized that I wasn't paying attention to type annotations for the returned object. I've marked five places in this PR where this return type annotation is missing, but there are many other functions/methods where it is missing; please review all functions that have been changed in this PR and ensure that they contain a return type annotation.
| return sorted(list(ret)) | ||
|
|
||
| def radius(self, base_nodes, max_hops): | ||
| def radius(self, base_nodes: Iterable[QubitLabel_in_Graph], max_hops: int): |
There was a problem hiding this comment.
Please also annotate the return type of the function.
| return sorted(list(ret)) | ||
|
|
||
| def connected_combos(self, possible_nodes, size): | ||
| def connected_combos(self, possible_nodes: list[QubitLabel_in_Graph], size: int): |
There was a problem hiding this comment.
Please also annotate the return type of the function.
| return bool(self._connectivity[j, i]) | ||
|
|
||
| def is_connected(self, node1, node2): | ||
| def is_connected(self, node1: QubitLabel_in_Graph, node2: QubitLabel_in_Graph): |
There was a problem hiding this comment.
Please also annotate the return type of the function.
| return self._predecessors[i, j] >= 0 | ||
|
|
||
| def has_edge(self, edge): | ||
| def has_edge(self, edge: tuple[QubitLabel_in_Graph, QubitLabel_in_Graph]): |
There was a problem hiding this comment.
Please also annotate the return type of the function.
| return self.is_directly_connected(edge[0], edge[1]) | ||
|
|
||
| def is_directly_connected(self, node1, node2): | ||
| def is_directly_connected(self, node1: QubitLabel_in_Graph, node2: QubitLabel_in_Graph): |
There was a problem hiding this comment.
Please also annotate the return type of the function.
- errorgenlabel.py: annotate mapper param in map_state_space_labels - qubitgraph.py: annotate common_graph params and add missing return type annotations across changed methods (radius, connected_combos, is_connected, has_edge, is_directly_connected, add_edge(s), remove_edge, shortest_path*, subgraph, resolve_relative_nodelabel, move_in_direction(s), is_connected_subgraph, etc.) - protectedarray.py: fix indices_to_protect and protected_index_mask annotations to match actual accepted inputs, and document the new behavior that in-place operators always raise ValueError - rpedata.py: annotate model_or_dataset and string_list_d in make_rpe_data_set - bootstrap.py: annotate input_model/target_model/verbosity/return_data in create_bootstrap_dataset and create_bootstrap_models; fix grammar in gauge_optimize_models docstring (determining -> determine) - errorgenspace.py: fix normalize()'s norm_order type hint (was Union[int, Literal['inf','-inf']], which doesn't match what numpy.linalg.norm accepts); also fix stale elemgen_basis/basis docstring parameter name mismatch in ErrorgenSpace.__init__
ndsieki
left a comment
ndsieki
left a comment
There was a problem hiding this comment.
Looks good to me. There are some return type annotations that could be added, but all of the provided type annotations appear to be correct.
… check. Also block raw memory access.
…overwrite the protected memory from a different view aliased to the same memory.
2 participants
Add type annotations to baseobjs to improve the maintainability of the code.