Skip to content
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

Fix issues raised by pylint #205

Merged
merged 24 commits into from
Oct 31, 2023
Merged

Fix issues raised by pylint #205

merged 24 commits into from
Oct 31, 2023

Conversation

purva-thakre
Copy link
Collaborator

@purva-thakre purva-thakre commented Oct 19, 2023

Description

Fixes #198

Todos

Notable points that this PR has either accomplished or will accomplish.

  • []

Questions

  • Question1

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #205 (0861b4a) into master (74f8b85) will increase coverage by 0.1%.
The diff coverage is 90.5%.

@@           Coverage Diff            @@
##           master    #205     +/-   ##
========================================
+ Coverage    95.9%   96.0%   +0.1%     
========================================
  Files         146     146             
  Lines        2961    2954      -7     
  Branches      725     728      +3     
========================================
- Hits         2840    2838      -2     
+ Misses         76      71      -5     
  Partials       45      45             
Files Coverage Δ
toqito/channel_metrics/fidelity_of_separability.py 100.0% <ø> (ø)
toqito/channel_ops/dual_channel.py 100.0% <ø> (ø)
toqito/channels/partial_trace.py 96.1% <100.0%> (+0.1%) ⬆️
toqito/channels/partial_transpose.py 100.0% <100.0%> (ø)
toqito/channels/realignment.py 92.8% <100.0%> (ø)
toqito/helper/npa_hierarchy.py 99.1% <100.0%> (ø)
toqito/matrices/pauli.py 100.0% <100.0%> (ø)
toqito/matrix_ops/inner_product.py 100.0% <ø> (ø)
toqito/matrix_ops/outer_product.py 100.0% <ø> (ø)
toqito/matrix_ops/vectors_from_gram_matrix.py 100.0% <100.0%> (ø)
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@purva-thakre purva-thakre force-pushed the pylint_errors branch 3 times, most recently from 572b622 to e9de123 Compare October 28, 2023 17:25
@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Oct 29, 2023

@vprusso I chose to ignore all the redefinition errors raised by pylint in 95dad08. Let me know if you would prefer I refactor those code lines in some way instead.

Edit: Same thing for the unsubscriptable errors in f9dca8b. Because a function allows for both int and array/list inputs, the easiest path was to disable them wherever possible.

Edit2: I think 8d59bbd was flagged as an error due to a bug in pylint.

@purva-thakre purva-thakre marked this pull request as ready for review October 29, 2023 23:03
@purva-thakre
Copy link
Collaborator Author

Ready for a review.

Copy link
Owner

@vprusso vprusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments, but overall this looks great! Excellent job, @purva-thakre !

toqito/channels/partial_transpose.py Outdated Show resolved Hide resolved
toqito/matrix_ops/vectors_from_gram_matrix.py Outdated Show resolved Hide resolved
@@ -1,10 +1,10 @@
"""Is matrix a positive definite matrix."""
import numpy as np

from toqito.matrix_props import is_hermitian
from toqito.matrix_props import is_hermitian # pylint: disable=unused-import
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this import is not used, we perhaps want to remove it.

Copy link
Collaborator Author

@purva-thakre purva-thakre Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This is a bug in pylint. We use this import but pylint thinks we are not using it.

Edit: This line is also causing issues in the module not found error discussed in this comment.

#210 (comment)

Maybe pylint is also catching the same issue here?

toqito/state_metrics/fidelity_of_separability.py Outdated Show resolved Hide resolved
toqito/state_metrics/fidelity_of_separability.py Outdated Show resolved Hide resolved
@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Oct 30, 2023

edit3: Same issue is flagged in a 3.10 run.
https://github.com/vprusso/toqito/actions/runs/6699061202/job/18202446701?pr=205#step:6:13


Edit2:Now, the changes made in the previous commit are flagged as problematic in a 3.11 run. Might be related to the deprecation warning fixed in #210

https://github.com/vprusso/toqito/actions/runs/6697622267/job/18197892428?pr=205#step:6:13


Edit: Fixed by a07e214

Apparently, this is raised by my local virtual environment using py311

************* Module matrices.gell_mann
toqito/matrices/gell_mann.py:120:8: R0204: Redefinition of gm_op type from .ndarray to scipy.sparse._csr.csr_matrix (redefined-variable-type)


Original Message:

3.10 and 3.11 workflows are catching different issues.

In 3.11, output of pylint workflow is as shown below:

************* Module toqito.matrices.gell_mann
toqito/matrices/gell_mann.py:120:0: I0021: Useless suppression of 'redefined-variable-type' (useless-suppression)
------------------------------------
Your code has been rated at 10.00/10
************* Module toqito.matrices.gell_mann
toqito/matrices/gell_mann.py:120:0: I0021: Useless suppression of 'redefined-variable-type' (useless-suppression)
------------------------------------
Your code has been rated at 10.00/10
************* Module toqito.matrices.gell_mann
toqito/matrices/gell_mann.py:120:0: I0021: Useless suppression of 'redefined-variable-type' (useless-suppression)
------------------------------------
Your code has been rated at 10.00/10

But for 3.10, it is successful maybe due to the following line in pylintrc.
image

@purva-thakre
Copy link
Collaborator Author

@vprusso Except for the two unresolved comments #205 (comment), #205 (comment) this is ready for another review.

I think they are closely related to the warnings raised by pytest.

Copy link
Owner

@vprusso vprusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Merge when ready!

@purva-thakre purva-thakre merged commit 438e183 into master Oct 31, 2023
3 checks passed
@purva-thakre purva-thakre deleted the pylint_errors branch October 31, 2023 03:26
purva-thakre added a commit that referenced this pull request Oct 31, 2023
* all pylint issues in tests/

* state_props:eof

* python install

* pylint userwarning

* remove exception and from future annotations

* import lines

* matrix_props/is_positive_def

* fixes Make parameter typing consistent in doc examples #209

* state_metrics/fid_of_sep

* channel_props/is_trace_preserving

* nonlocalgame

* useless suppression

* quick fixes

* all redefinition disabled

* all unsubscriptable object

* possibly unbalanced tuples

* unnecessary-list-index-lookup

* black reformatter no pylint issues

* black reformatter pylint issues

* test useless supression

* Fixes #205 (comment)

* Fixes #205 (comment)

* Fixes #205 (comment)

* fixes #205 (comment) + black reformatter
purva-thakre added a commit that referenced this pull request Oct 31, 2023
purva-thakre added a commit that referenced this pull request Nov 9, 2023
* test_channel_metrics

* fix #205 (comment)

* scipy/sparse find_common_type deprecation

* ignore warnings related to cvxpy

* try ignore all cvxpy warnings

* try: ignore a line in a third party package

* try whole path

* try: omit virtualenvs

* add coveragerc to pytest path

* upgradepytest cov version

* remove coverage

* numpy: Ensure you extract a single element

* upgrade coverage

* Revert "scipy/sparse find_common_type deprecation"

This reverts commit fb9d3a0.

* update toml

* comment out matsumoto fidelity failure

* test: don't ignore cvxpy

* pylint + cleanup

* no else return

* upgrade scipy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint errors
2 participants