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 typing errors and set up mypy workflow action #176

Merged
merged 72 commits into from
May 1, 2022
Merged

Conversation

bwohlberg
Copy link
Collaborator

@bwohlberg bwohlberg commented Jan 13, 2022

Fix typing errors and set up mypy workflow action

To check a single file:

mypy --follow-imports=skip --ignore-missing-imports <filename>

The typing tests are now passing, with some significant limitations, as reflected in the command

mypy --follow-imports=skip --ignore-missing-imports  --exclude "(numpy|test)" scico

Most significantly, this excludes testing of scico.numpy, which results in 100s of typing errors, primarily due to unrecognized functions (which are dynamically constructed) in that module. Despite these limitations, I propose that we merge this PR now so that future changes are subjected to at least some typing testing. Follow-on projects would be:

  1. Figure out how to annotate a dynamically generated module so that the typing system is aware of the dynamically constructed functions.
  2. Revisit the numerous lines of code marked as # type: ignore to determine whether the error that led to its insertion can instead be resolved by improved type annotation or use of type guards.
  3. Review class structure with a view to minimization violations of the Liskov substitution principle (e.g. see issues with prox method).

@bwohlberg bwohlberg added the developer Developer environment: issues related to CI, git, etc. label Jan 13, 2022
@bwohlberg bwohlberg linked an issue Jan 14, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #176 (61779ff) into main (4381be5) will increase coverage by 0.07%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   94.07%   94.15%   +0.07%     
==========================================
  Files          49       49              
  Lines        3241     3267      +26     
==========================================
+ Hits         3049     3076      +27     
+ Misses        192      191       -1     
Flag Coverage Δ
unittests 94.15% <96.22%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/functional/_functional.py 92.95% <0.00%> (+2.54%) ⬆️
scico/numpy/__init__.py 100.00% <ø> (ø)
scico/numpy/blockarray.py 97.33% <ø> (ø)
scico/solver.py 98.52% <ø> (ø)
scico/util.py 90.90% <77.77%> (ø)
scico/linop/_stack.py 90.32% <88.23%> (-0.59%) ⬇️
scico/_flax.py 96.92% <100.00%> (ø)
scico/_generic_operators.py 91.92% <100.00%> (+0.11%) ⬆️
scico/_version.py 84.61% <100.00%> (ø)
scico/diagnostics.py 88.05% <100.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4381be5...61779ff. Read the comment docs.

@bwohlberg bwohlberg marked this pull request as ready for review April 24, 2022 18:28
scico/optimize/admm.py Show resolved Hide resolved
scico/optimize/admm.py Show resolved Hide resolved
@@ -59,13 +59,15 @@ def radial_transverse_frequency(
raise ValueError("Invalid input dimensions; must be 1 or 2")

if np.isscalar(dx):
dx = (dx,) * ndim
dx = (dx,) * ndim # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly this could be achieved via a type guarding version of isscalar https://mypy.readthedocs.io/en/latest/type_narrowing.html#user-defined-type-guards

scico/typing.py Outdated Show resolved Hide resolved
@bwohlberg
Copy link
Collaborator Author

Remaining issues to be dealt with notes as #284.

@bwohlberg bwohlberg merged commit 157f2ba into main May 1, 2022
@bwohlberg bwohlberg deleted the brendt/typing branch May 1, 2022 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Developer environment: issues related to CI, git, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type checking in workflows
3 participants