Skip to content

Commit

Permalink
mypy compliance (#1721)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
Zeitsperre authored May 1, 2024
2 parents 08bbc42 + ce99186 commit 3743ef9
Show file tree
Hide file tree
Showing 41 changed files with 1,389 additions and 1,011 deletions.
5 changes: 4 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ Breaking changes
* The previously deprecated function ``xclim.ensembles.change_significance`` has been removed. (:pull:`1737`).
* Indicators ``snw_season_length`` and ``snd_season_length`` have been modified, see above.
* The `hargeaves85`/`hg85` method for the ``potential_evapotranspiration`` indicator and indice has been modified for precision and consistency with recent academic literature. (:issue:`1710`, :pull:`1723`).

* The `__getitem__` method of ``xclim.core.indicator.Parameter`` instances has been removed. Accessing members of ``Parameters`` now uniquely uses dot notation. (:pull:`1721`).
* The obsolete function wrapper for generating Indicators ``xclim.core.utils.wrapped_partial`` has been removed. (:pull:`1721`).

Bug fixes
^^^^^^^^^
Expand All @@ -39,6 +40,7 @@ Bug fixes
* Fixed "agreement fraction" in ``robustness_fractions`` to distinguish between negative change and no change. Added "negative" and "changed negative" fractions (:issue:`1690`, :pull:`1711`).
* ``make_criteria`` now skips columns with NaNs across all realizations. (:pull:`1713`).
* Fixed bug QuantileDeltaMapping adjustment not working for seasonal grouping (:issue:`1704`, :pull:`1716`).
* The codebase has been adjusted to address several (~400) `mypy`-related errors attributable to inaccurate function call signatures and variable name shadowing. (:issue:`1719`, :pull:`1721`).

Internal changes
^^^^^^^^^^^^^^^^
Expand All @@ -48,6 +50,7 @@ Internal changes
* Added the `tox-gh` dependency to the development installation recipe. This will soon be required for running the `tox` test ensemble on GitHub Workflows. (:pull:`1709`).
* Added the `vulture` static code analysis tool for finding dead code to the development dependency list and linters (makefile, tox and pre-commit hooks). (:pull:`1717`).
* Added error message when using `xclim.indices.stats.dist_method` with `nnlf` and included note in docstring. (:issue:`1683`, :pull:`1714`).
* PEP8 rule `N802` is now enabled in the `ruff` formatter. Function names should follow `Snake case <https://en.wikipedia.org/wiki/Snake_case>`_, with rare exceptions. (:pull:`1721`).

v0.48.2 (2024-02-26)
--------------------
Expand Down
1 change: 0 additions & 1 deletion docs/notebooks/extendxclim.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,6 @@
"outputs": [],
"source": [
"from xclim.core.indicator import build_indicator_module, registry\n",
"from xclim.core.utils import wrapped_partial\n",
"\n",
"mapping = dict(\n",
" egg_cooking_season=registry[\"MAXIMUM_CONSECUTIVE_WARM_DAYS\"](\n",
Expand Down
1 change: 1 addition & 0 deletions environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ dependencies:
- nc-time-axis
- netCDF4 >=1.4
- notebook
- pandas-stubs
- platformdirs
- pooch
- pre-commit
Expand Down
22 changes: 10 additions & 12 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ dev = [
"nbqa",
"nbval",
"netCDF4 >=1.4",
"pandas-stubs>=2.2",
"platformdirs >=3.2",
"pre-commit >=2.9",
"pybtex",
Expand Down Expand Up @@ -153,7 +154,7 @@ values = [

[tool.codespell]
skip = 'xclim/data/*.json,docs/_build,docs/notebooks/xclim_training/*.ipynb,docs/references.bib,__pycache__,*.nc,*.png,*.gz,*.whl'
ignore-words-list = "absolue,astroid,bloc,bui,callendar,degreee,environnement,hanel,inferrable,lond,nam,nd,ressources,vas"
ignore-words-list = "absolue,astroid,bloc,bui,callendar,degreee,environnement,hanel,inferrable,lond,nam,nd,ressources,sie,vas"

[tool.coverage.run]
relative_files = true
Expand Down Expand Up @@ -207,24 +208,20 @@ python_version = 3.9
show_error_codes = true
warn_return_any = true
warn_unused_configs = true
plugins = ["numpy.typing.mypy_plugin"]

[[tool.mypy.overrides]]
module = [
"boltons.*",
"bottleneck.*",
"cftime.*",
"clisops.core.subset.*",
"dask.*",
"lmoments3.*",
"matplotlib.*",
"jsonpickle.*",
"numba.*",
"numpy.*",
"pandas.*",
"pint.*",
"pytest_socket.*",
"SBCK.*",
"scipy.*",
"sklearn.cluster.*",
"xarray.*",
"sklearn.*",
"statsmodels.*",
"yamale.*",
"yaml.*"
]
ignore_missing_imports = true
Expand Down Expand Up @@ -273,6 +270,7 @@ select = [
"D",
"E",
"F",
"N802",
"W"
]

Expand All @@ -299,7 +297,7 @@ max-complexity = 20

[tool.ruff.lint.per-file-ignores]
"docs/*.py" = ["D100", "D101", "D102", "D103"]
"tests/*.py" = ["D100", "D101", "D102", "D103"]
"tests/*.py" = ["D100", "D101", "D102", "D103", "N802"]
"xclim/**/__init__.py" = ["F401", "F403"]
"xclim/core/indicator.py" = ["D214", "D405", "D406", "D407", "D411"]
"xclim/core/locales.py" = ["E501", "W505"]
Expand Down
36 changes: 19 additions & 17 deletions tests/test_indicators.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def test_opt_vars(tasmin_series, tasmax_series):
tx = tasmax_series(np.zeros(365))

multiOptVar(tasmin=tn, tasmax=tx)
assert multiOptVar.parameters["tasmin"]["kind"] == InputKind.OPTIONAL_VARIABLE
assert multiOptVar.parameters["tasmin"].kind == InputKind.OPTIONAL_VARIABLE


def test_registering():
Expand Down Expand Up @@ -265,8 +265,10 @@ def test_module():
"""Translations are keyed according to the module where the indicators are defined."""
assert atmos.tg_mean.__module__.split(".")[2] == "atmos"
# Virtual module also are stored under xclim.indicators
assert xclim.indicators.cf.fg.__module__ == "xclim.indicators.cf"
assert xclim.indicators.icclim.GD4.__module__ == "xclim.indicators.icclim"
assert xclim.indicators.cf.fg.__module__ == "xclim.indicators.cf" # noqa: F821
assert (
xclim.indicators.icclim.GD4.__module__ == "xclim.indicators.icclim"
) # noqa: F821


def test_temp_unit_conversion(tas_series):
Expand Down Expand Up @@ -377,7 +379,7 @@ def test_multiindicator(tas_series):
compute=uniindtemp_compute,
)
with pytest.raises(ValueError, match="Indicator minmaxtemp4 was wrongly defined"):
tmin, tmax = ind(tas, freq="YS")
_tmin, _tmax = ind(tas, freq="YS")


def test_missing(tas_series):
Expand Down Expand Up @@ -480,7 +482,7 @@ def test_all_parameters_understood(official_indicators):
for identifier, ind in official_indicators.items():
indinst = ind.get_instance()
for name, param in indinst.parameters.items():
if param["kind"] == InputKind.OTHER_PARAMETER:
if param.kind == InputKind.OTHER_PARAMETER:
problems.add((identifier, name))
# this one we are ok with.
if problems - {
Expand Down Expand Up @@ -587,15 +589,15 @@ def test_parsed_doc():
assert "tas" in xclim.atmos.liquid_precip_accumulation.parameters

params = xclim.atmos.drought_code.parameters
assert params["tas"]["description"] == "Noon temperature."
assert params["tas"]["units"] == "[temperature]"
assert params["tas"]["kind"] is InputKind.VARIABLE
assert params["tas"]["default"] == "tas"
assert params["snd"]["default"] is None
assert params["snd"]["kind"] is InputKind.OPTIONAL_VARIABLE
assert params["snd"]["units"] == "[length]"
assert params["season_method"]["kind"] is InputKind.STRING
assert params["season_method"]["choices"] == {"GFWED", None, "WF93", "LA08"}
assert params["tas"].description == "Noon temperature."
assert params["tas"].units == "[temperature]"
assert params["tas"].kind is InputKind.VARIABLE
assert params["tas"].default == "tas"
assert params["snd"].default is None
assert params["snd"].kind is InputKind.OPTIONAL_VARIABLE
assert params["snd"].units == "[length]"
assert params["season_method"].kind is InputKind.STRING
assert params["season_method"].choices == {"GFWED", None, "WF93", "LA08"}


def test_default_formatter():
Expand Down Expand Up @@ -655,14 +657,14 @@ def test_input_dataset(open_dataset):
ds = open_dataset("ERA5/daily_surface_cancities_1990-1993.nc")

# Use defaults
out = xclim.atmos.daily_temperature_range(freq="YS", ds=ds)
_ = xclim.atmos.daily_temperature_range(freq="YS", ds=ds)

# Use non-defaults (inverted on purpose)
with xclim.set_options(cf_compliance="log"):
out = xclim.atmos.daily_temperature_range("tasmax", "tasmin", freq="YS", ds=ds)
_ = xclim.atmos.daily_temperature_range("tasmax", "tasmin", freq="YS", ds=ds)

# Use a mix
out = xclim.atmos.daily_temperature_range(tasmax=ds.tasmax, freq="YS", ds=ds)
_ = xclim.atmos.daily_temperature_range(tasmax=ds.tasmax, freq="YS", ds=ds)

# Inexistent variable:
dsx = ds.drop_vars("tasmin")
Expand Down
26 changes: 0 additions & 26 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
# Test for utils
from __future__ import annotations

from inspect import signature

import numpy as np
import xarray as xr

Expand All @@ -12,7 +10,6 @@
ensure_chunk_size,
nan_calc_percentiles,
walk_map,
wrapped_partial,
)
from xclim.testing.helpers import test_timeseries as _test_timeseries

Expand All @@ -24,29 +21,6 @@ def test_walk_map():
assert o["b"]["c"] == 0


def test_wrapped_partial():
def func(a, b=1, c=1):
"""Docstring"""
return (a, b, c)

newf = wrapped_partial(func, b=2)
assert list(signature(newf).parameters.keys()) == ["a", "c"]
assert newf(1) == (1, 2, 1)

newf = wrapped_partial(func, suggested=dict(c=2), b=2)
assert list(signature(newf).parameters.keys()) == ["a", "c"]
assert newf(1) == (1, 2, 2)
assert newf.__doc__ == func.__doc__

def func(a, b=1, c=1, **kws): # pylint: disable=function-redefined
"""Docstring"""
return a, b, c

newf = wrapped_partial(func, suggested=dict(c=2), a=2, b=2)
assert list(signature(newf).parameters.keys()) == ["c", "kws"]
assert newf() == (2, 2, 2)


def test_ensure_chunk_size():
da = xr.DataArray(np.zeros((20, 21, 20)), dims=("x", "y", "z"))

Expand Down
25 changes: 10 additions & 15 deletions xclim/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@

from __future__ import annotations

try:
from importlib.resources import files as _files
except ImportError:
from importlib_resources import files as _files
import importlib.resources as _resources

from xclim import indices
from xclim.core import units # noqa
Expand All @@ -19,15 +16,13 @@
__version__ = "0.48.3-dev.15"


_module_data = _files("xclim.data")
with _resources.as_file(_resources.files("xclim.data")) as _module_data:
# Load official locales
for filename in _module_data.glob("??.json"):
# Only select <locale>.json and not <module>.<locale>.json
load_locale(filename, filename.stem)

# Load official locales
for filename in _module_data.glob("??.json"):
# Only select <locale>.json and not <module>.<locale>.json
load_locale(filename, filename.stem)


# Virtual modules creation:
build_indicator_module_from_yaml(_module_data / "icclim", mode="raise")
build_indicator_module_from_yaml(_module_data / "anuclim", mode="raise")
build_indicator_module_from_yaml(_module_data / "cf", mode="raise")
# Virtual modules creation:
build_indicator_module_from_yaml(_module_data / "icclim", mode="raise")
build_indicator_module_from_yaml(_module_data / "anuclim", mode="raise")
build_indicator_module_from_yaml(_module_data / "cf", mode="raise")
14 changes: 7 additions & 7 deletions xclim/analog.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ def spatial_analogs(
# Create the target DataArray
# drop any (sub-)index along "dist_dim" that could conflict with target, and rename it.
# The drop is the simplest solution that is compatible with both xarray <=2022.3.0 and >2022.3.1
candidates = candidates.to_array("_indices", "candidates").rename(
candidate_array = candidates.to_array("_indices", "candidates").rename(
{dist_dim: "_dist_dim"}
)
if isinstance(candidates.indexes["_dist_dim"], pd.MultiIndex):
candidates = candidates.drop_vars(
["_dist_dim"] + candidates.indexes["_dist_dim"].names,
if isinstance(candidate_array.indexes["_dist_dim"], pd.MultiIndex):
candidate_array = candidate_array.drop_vars(
["_dist_dim"] + candidate_array.indexes["_dist_dim"].names,
# in xarray <= 2022.3.0 the sub-indexes are not listed as separate coords,
# instead, they are dropped when the multiindex is dropped.
errors="ignore",
Expand All @@ -78,16 +78,16 @@ def spatial_analogs(
f"Method `{method}` is not implemented. Available methods are: {','.join(metrics.keys())}."
) from e

if candidates.chunks is not None:
candidates = candidates.chunk({"_indices": -1})
if candidate_array.chunks is not None:
candidate_array = candidate_array.chunk({"_indices": -1})
if target_array.chunks is not None:
target_array = target_array.chunk({"_indices": -1})

# Compute dissimilarity
diss = xr.apply_ufunc(
metric_func,
target_array,
candidates,
candidate_array,
input_core_dims=[(dist_dim, "_indices"), ("_dist_dim", "_indices")],
output_core_dims=[()],
vectorize=True,
Expand Down
10 changes: 6 additions & 4 deletions xclim/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
from xclim.testing.helpers import TESTDATA_BRANCH, populate_testing_data
from xclim.testing.utils import _default_cache_dir, publish_release_notes, show_versions

distributed = False
try:
from dask.distributed import Client, progress # pylint: disable=ungrouped-imports
from dask.distributed import Client, progress

distributed = True
except ImportError:
# Distributed is not a dependency of xclim
Client = None
progress = None
pass


def _get_indicator(indicator_name):
Expand Down Expand Up @@ -432,7 +434,7 @@ def cli(ctx, **kwargs):
kwargs["input"] = kwargs["input"][0]

if kwargs["dask_nthreads"] is not None:
if Client is None:
if not distributed:
raise click.BadOptionUsage(
"dask_nthreads",
"Dask's distributed scheduler is not installed, only the "
Expand Down
Loading

0 comments on commit 3743ef9

Please sign in to comment.