Type annotations in reporting - #720
Conversation
… then never using it.
…or similar. At this time, I believe that Results = protocol.ProtocolResults or a class which inherits from that.
ndsieki
left a comment
ndsieki
left a comment
There was a problem hiding this comment.
Many type annotations are missing for function parameters. I've marked them explicitly in many cases.
Most functions/methods are also missing an annotation of the return type. I only marked the first few instances of this missing return type; in general, it should be added for every function/method, including those where I didn't leave a comment.
I've marked every case where I observed that the provided type annotation was incorrect. There were not many of those cases.
|
|
||
|
|
||
| def entanglement_fidelity(a, b, mx_basis): | ||
| def entanglement_fidelity(a, b, mx_basis: BasisLike): |
There was a problem hiding this comment.
Please add a type annotation for a and b.
There was a problem hiding this comment.
Please apply this comment to all other functions foo(a, b, mx_basis: BasisLike).
|
|
||
|
|
||
| def errorgen_and_projections(errgen, mx_basis): | ||
| def errorgen_and_projections(errgen, mx_basis: BasisLike): |
There was a problem hiding this comment.
Please add type annotation for errgen.
|
|
||
|
|
||
| def vec_as_stdmx(vec, mx_basis): | ||
| def vec_as_stdmx(vec, mx_basis: BasisLike): |
There was a problem hiding this comment.
Please add a type annotation for vec.
|
|
||
|
|
||
| def vec_as_stdmx_eigenvalues(vec, mx_basis): | ||
| def vec_as_stdmx_eigenvalues(vec, mx_basis: BasisLike): |
There was a problem hiding this comment.
Please add a type annotation for vec.
…dance with Noah's comments.
ndsieki
left a comment
ndsieki
left a comment
There was a problem hiding this comment.
There are a lot of comments because there are a lot of files. I found minimal cases where an annotation was actually incorrect. There are cases where the annotation is imprecise (e.g., str instead of Literal[...], or Any when a type is clarified in the docstring) and I've indicated those cases. I don't fully trust the docstrings for some functions but I've used them as a point of reference where relevant.
|
|
||
| def __init__(self, ws, xs, circuits_by_x, model_by_x, dataset_by_x, objfn_builder='logl', | ||
| x_label='L', np_by_x=None, comm=None, wildcard=None, mdc_stores=None): | ||
| def __init__(self, ws: _Workspace, xs: Sequence[Any], circuits_by_x: Sequence[Union[_CircuitList, Sequence[_Circuit]]], model_by_x: Sequence[Optional[_Model]], dataset_by_x: Union[_DataSet, Sequence[_DataSet]], objfn_builder: Union[str, _ObjectiveFunctionBuilder] = 'logl', |
There was a problem hiding this comment.
For xs, docstring indicates Sequence[int]; please update the annotation if docstring is correct.
objfn_builder has literal options in the docstring as well, please incorporate if appropriate.
| """ | ||
|
|
||
| def __init__(self, ws, circuit_lists, titles, num_cols=1, common_title=None): | ||
| def __init__(self, ws: _Workspace, circuit_lists: Sequence[Any], titles: Union[str, Sequence[str]], num_cols: int = 1, common_title: Optional[str] = None) -> None: |
There was a problem hiding this comment.
Please update the circuit_lists annotation with the more precise annotation found in other classes in this file.
| def __init__(self, ws, metric, models, target_models, titles, | ||
| rowtitles=None, table_title=None, op_label=None, | ||
| confidence_region_info=None): | ||
| def __init__(self, ws: _Workspace, metric: AVAILABLE_METRICS, models: Sequence[Any], target_models: Sequence[Any], titles: Sequence[str], |
There was a problem hiding this comment.
Please update the Any type annotations with more precise Model annotations.
|
|
||
| def __init__(self, ws, model_dim, projection_type, | ||
| projection_basis): | ||
| def __init__(self, ws: _Workspace, model_dim: int, projection_type: str, |
There was a problem hiding this comment.
Please add literal type annotations for projection_type and projection_basis as found in the docstring.
| """ | ||
|
|
||
| def __init__(self, ws, profiler, sort_by="time"): | ||
| def __init__(self, ws: _Workspace, profiler: Optional[_Profiler], sort_by: str = "time") -> None: |
There was a problem hiding this comment.
Please add Literal annotation for sort_by following the docstring.
3 participants
Improving maintainability by adding more type annotations to the reporting modules of the code.