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

Type annotations, mypy config, pydoclint setup, and other changes #1660

Merged
merged 9 commits into from
Aug 29, 2024

Conversation

VeckoTheGecko
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko commented Aug 20, 2024

Overview

This PR introduces typing to the Parcels codebase, providing:

  • useful information to users regarding the types of parameters
  • useful information to developers regarding the types of variables
  • a mechanism to bring types into active consideration, where complicated typing can be a "code smell" indicating a deeper problem with the code

The coverage for this isn't 100% yet (as doing so is quite difficult with the current codebase), but as we refactor code it will get easier to increase our coverage.

Changes

Typing:

  • Add mypy to check types (also with a CI workflow)
  • Add mypy config, with list of files to check for types (this list will be included). Note that this list is only the source files (i.e., entrypoint for mypy). Types with imported parcels functions/methods are also checked
  • Add typing to various files
  • Introduce a _typing.py file to define types used throughout Parcels

Other:

  • add pydoclint tooling to pre-commit to help capture docstring errors not picked up by Ruff
  • add _compat.py file to help with compatibility between installations when using mpi
  • Implement some suggestions from repo-review tool
    • Notably:
      • concurrency with GHA (i.e., repeated pushes to a branch will cancel already running workflows on prior commits)
      • dependabot to automatically open PRs to update GHA workflow versions monthly
  • Fix unit-test badge in README and add Ruff badge
  • Clean up some code involving casting to arrays

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 98.10427% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
parcels/tools/interpolation_utils.py 90.90% 2 Missing ⚠️
parcels/field.py 97.77% 1 Missing ⚠️
parcels/fieldset.py 97.82% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (997e3f8) and HEAD (6fa0515). Click for more details.

HEAD has 8 uploads less than BASE
Flag BASE (997e3f8) HEAD (6fa0515)
unit-tests 10 5
integration-tests 6 3
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1660      +/-   ##
==========================================
- Coverage   86.40%   81.03%   -5.38%     
==========================================
  Files          67       69       +2     
  Lines       11746    13118    +1372     
  Branches        0      125     +125     
==========================================
+ Hits        10149    10630     +481     
- Misses       1597     2451     +854     
- Partials        0       37      +37     
Flag Coverage Δ
integration-tests 64.86% <89.47%> (+0.02%) ⬆️
mypy 14.90% <75.96%> (?)
unit-tests 80.87% <92.48%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
parcels/_compat.py 100.00% <100.00%> (ø)
parcels/_typing.py 100.00% <100.00%> (ø)
parcels/compilation/codecompiler.py 91.42% <100.00%> (-4.32%) ⬇️
parcels/compilation/codegenerator.py 78.61% <100.00%> (-6.30%) ⬇️
parcels/fieldfilebuffer.py 70.01% <100.00%> (-3.32%) ⬇️
parcels/grid.py 73.02% <100.00%> (-18.86%) ⬇️
parcels/interaction/interactionkernel.py 78.44% <100.00%> (+2.41%) ⬆️
parcels/kernel.py 76.70% <100.00%> (-9.69%) ⬇️
parcels/particledata.py 71.05% <100.00%> (-10.63%) ⬇️
parcels/particlefile.py 77.41% <100.00%> (-17.95%) ⬇️
... and 8 more

... and 10 files with indirect coverage changes

_compat.py file was mentioned in https://learn.scientific-python.org/development/patterns/backports/#placement-in-a-file . Thought it would be useful to consolidate MPI stuff
Need to investigate the use of `TYPE_CHECKING` as it causes runtime `NameErrors` even when only used as annotations.
@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review August 27, 2024 12:04
- Add concurrency for workflows (so repeated pushes on the same branch cancel previous workflow run)
- Add dependabot for GHA version updating
- Add prettier as formatter (removing Biome as it didn't format the needed files)
- --show-fixes in ruff config

Used https://learn.scientific-python.org/development/guides/repo-review/ to highlight suggested changes in project configuration. Some items (upload-artifact version change, nox/tox integration, spell checker, pytest options) weren't included as they add  maintainence burden beyond their benefits, or hinder development (codespell).
@VeckoTheGecko VeckoTheGecko changed the title [WIP] Type annotations, mypy config, pydoclint setup Type annotations, mypy config, pydoclint setup, and other changes Aug 27, 2024
parcels/_typing.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
parcels/_typing.py Outdated Show resolved Hide resolved
parcels/_typing.py Outdated Show resolved Hide resolved
parcels/_typing.py Outdated Show resolved Hide resolved
parcels/_typing.py Show resolved Hide resolved
parcels/field.py Outdated Show resolved Hide resolved
parcels/fieldset.py Show resolved Hide resolved
parcels/grid.py Show resolved Hide resolved
parcels/kernel.py Show resolved Hide resolved
parcels/tools/converters.py Outdated Show resolved Hide resolved
@VeckoTheGecko VeckoTheGecko merged commit 85b6cde into master Aug 29, 2024
13 checks passed
@VeckoTheGecko VeckoTheGecko deleted the v/type-annotations branch August 29, 2024 14:38
@VeckoTheGecko
Copy link
Contributor Author

Fixes #1653 . Refactoring to reduce code duplication in fieldset.py will be done in a future PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants