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

mypy compliance #1721

Merged
merged 51 commits into from
May 1, 2024
Merged

mypy compliance #1721

merged 51 commits into from
May 1, 2024

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Apr 18, 2024

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Addresses several issues (around 250/1250 errors) associated with (re-)casting of variables and output typing.
  • The Indicator.__getitem__ method has been retired (to reduce instances of class methods returning Any)
  • PEP8 rule N802 is now enforced (with noqa exceptions, mostly for functions based on sfcWind variable).
  • The wrapped_partial function wrapper has been removed.

Does this PR introduce a breaking change?

Yes. The Indicator __getitem__ method has been removed. This has downstream impacts on finch and possibly other projects. Additionally, the obsolete wrapped_partial function (was previously used in Indicator construction) has also been removed.

Other information:

@aulemahal

We currently use the Quantified type alias in almost all the threshold-based indicators to specify that this variable should conform to something resembling a string with units (str | pint.Quantity), but this alias also includes xarray.DataArray. Does this mean that we can pass (or would like to enable passing) DataArrays of grids denoting thresholds? If not, we may need to create a new, more constrained custom variable (str2pint often takes variables of this type but does not support pint.Quantity or xarray.DataArray). Solving this issue alone will address +250 errors.

@Zeitsperre Zeitsperre added help wanted Extra attention is needed standards / conventions Suggestions on ways forward labels Apr 18, 2024
@Zeitsperre Zeitsperre self-assigned this Apr 18, 2024
@github-actions github-actions bot added sdba Issues concerning the sdba submodule. indicators Climate indices and indicators labels Apr 18, 2024
xclim/__init__.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the API Interfacing and User Concerns label Apr 18, 2024
@Zeitsperre
Copy link
Collaborator Author

Zeitsperre commented Apr 22, 2024

There seems to be a handful of trip-falls that we keep running into:

  1. Many of the decorators that we employ do not constrain the types of variables passed in/out of them; This makes it so that regardless of the typing identified by a decorated function, the end results will always return Any. This can be fixed by using -> TypeVar[...] signatures.
  2. Mentioned above, Quantified is used too often in places where it doesn't actually work (some functions demand str-like variables, but claim to support Quantified, which includes DataArray). This could become a breaking change but would address many errors.

Changes as of this comment have us hovering at around 1050 errors total.

@Zeitsperre
Copy link
Collaborator Author

Zeitsperre commented Apr 22, 2024

Additional comment:

Due to the way that numpy is typed, any time that we pass xarray.DataArray to a numpy.ufunc-like operation (e.g. np.logical_and(da1, da2)) we need to specify that the output is still a xarray.DataArray (using: output: xarray.DataArray; expected output is an ndarray otherwise).

Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

This looks good, but there are a lot of cases where it seems an annotation shouldn't be necessary ? I've commented on a few, but not all.

xclim/indices/_multivariate.py Show resolved Hide resolved
xclim/indices/_multivariate.py Outdated Show resolved Hide resolved
xclim/indices/_multivariate.py Outdated Show resolved Hide resolved
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/indices/_threshold.py Outdated Show resolved Hide resolved
xclim/core/units.py Outdated Show resolved Hide resolved
@Zeitsperre
Copy link
Collaborator Author

This is just missing the updates to the changelog about breaking changes, but I think we're better off now. Lots of small improvements.

@Zeitsperre Zeitsperre requested a review from aulemahal May 1, 2024 19:35
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

Thanks @Zeitsperre! This was a lot of work.

I have a last conservative comment, but this looks like a very good start for type-checked xclim.

xclim/indices/_conversion.py Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label May 1, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the docs Improvements to documenation label May 1, 2024
@Zeitsperre Zeitsperre merged commit 3743ef9 into main May 1, 2024
19 checks passed
@Zeitsperre Zeitsperre deleted the mypy-pylint branch May 1, 2024 21:44
@juliettelavoie juliettelavoie mentioned this pull request Aug 2, 2024
5 tasks
juliettelavoie added a commit that referenced this pull request Aug 5, 2024
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [ ] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fixes #xyz
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGELOG.rst has been updated (with summary of main changes)
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added

### What kind of change does this PR introduce?

* I am casting `high` to be a float, not an array. In the new version of
dask, if `high` is an array, there are memory problems and we are unable
to run jitter_under on the ESPO regions.
* I see that it was made explictly an array in #1721. @Zeitsperre do you
remember why ?

### Does this PR introduce a breaking change?
no

### Other information:
This is only a problem when using dask.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interfacing and User Concerns approved Approved for additional tests docs Improvements to documenation help wanted Extra attention is needed indicators Climate indices and indicators sdba Issues concerning the sdba submodule. standards / conventions Suggestions on ways forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants