-
Notifications
You must be signed in to change notification settings - Fork 55
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
Type checking for channel_metrics
#623
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #623 +/- ##
======================================
Coverage 98.1% 98.1%
======================================
Files 162 162
Lines 3112 3115 +3
Branches 759 759
======================================
+ Hits 3054 3057 +3
Misses 37 37
Partials 21 21 ☔ View full report in Codecov by Sentry. |
|
||
[tool.mypy] | ||
disable_error_code = ["import-untyped"] |
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.
added this because not all libraries use type annotations.
Skipping analyzing "picos": module is installed, but missing library stubs or py.typed marker
@@ -178,7 +179,7 @@ def fidelity_of_separability( | |||
problem.add_constraint((picos.I(dim_r) @ sym_choi) * choi * (picos.I(dim_r) @ sym_choi) == choi) | |||
|
|||
# PPT condition on Choi state | |||
sys = [] | |||
sys = [] # type: List[int] |
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 is an inline type annotation as requested by mypy
Need type annotation for "sys" (hint: "sys: list[<type>] = ...") [var-annotated]
I think the type annotation for It is https://www.cvxpy.org/api_reference/cvxpy.atoms.affine.html#partial-transpose To Do: Understand if I need to raise this in the cvxpy repo. |
dual_as_array = np.asarray(dual_channel(phi)) | ||
return completely_bounded_trace_norm.completely_bounded_trace_norm(dual_as_array) |
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.
Don't know if this is a best practice hack.
Due to dual_channel
, the return type of dual_channel(phi)
is np.ndarray | List[List[np.ndarray]]
when completely_bounded_trace_norm
expects just a np.ndarray
. So, added an additional step of converting dual_channel(phi)
.
If completely_bounded_trace_norm.completely_bounded_trace_norm
was not specified, mypy failed with toqito/channel_metrics/completely_bounded_spectral_norm.py:35: error: Module not callable [operator]
but python could not import this function to run unit tests.
@@ -64,7 +64,7 @@ def completely_bounded_trace_norm(phi: np.ndarray) -> float: | |||
a_var = cp.bmat([[y0, -phi], [-phi.conj().T, y1]]) | |||
constraints += [a_var >> 0] | |||
objective = cp.Minimize( | |||
cp.norm(cp.partial_trace(y0, dims=(dim, dim), axis=1)) + cp.norm(cp.partial_trace(y1, dims=(dim, dim), axis=1)) | |||
cp.norm(cp.partial_trace(y0, dims=(dim, dim), axis=1)) + cp.norm(cp.partial_trace(y1, dims=(dim, dim), axis=1)) # type: ignore |
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.
temporarily ignoring this line due to #623 (comment)
Description
Related to #299
We use
--explicit-package-bases
here to disable mypy errors due to 2 files of the same name existing in different modules.Changes
Right now
~/toqito$ mypy toqito/channel_metrics --explicit-package-bases
is failing due to the following. These are mostly due to incompatibilities with type annotations in cvxpy compared to how we use them.Will need to refactor portions of:
diamon_norm.py
:toqito/toqito/channel_metrics/diamond_norm.py
Line 102 in 1122a6e
Here,
[dim_squared, dim_Squared]
is of typeList[int]
whereascxpy.Parameter
expectsshape: int | tuple[int, ...]
https://www.cvxpy.org/api_reference/cvxpy.expressions.html#parameter
completely_bounded_trace_norm.py
Same as the previous item, we use an incompatible type compared to what's expected by cvpy type annotation. It expects
tuple[int]
but we usetuple[int. int]
.toqito/toqito/channel_metrics/completely_bounded_trace_norm.py
Line 67 in 1122a6e
https://www.cvxpy.org/api_reference/cvxpy.atoms.affine.html#partial-trace
completely_bounded_spectral_norm.py
For some reason,
completely_bounded_trace_norm
ordual_channel
shows up as a module not callable.toqito/toqito/channel_metrics/completely_bounded_spectral_norm.py
Line 35 in 1122a6e
If I specify the full path for
completely_bounded_trace_norm
ascompletely_bounded_trace_norm.completely_bounded_trace_norm
, mypy fails with the following error:Checklist
Before marking your PR ready for review, make sure you checked the following locally. If this is your first PR, you might be notified of some workflow failures after a maintainer has approved the workflow jobs to be run on your PR.
Additional information is available in the documentation.
ruff
for errors related to code style and formatting.pytest
.Sphinx
build can be checked locally for any failures related to your PRlinkcheck
to check for broken links in the documentationdoctest
to verify the examples in the function docstrings work as expected.