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 time indexer encoder and parser in fitting standardized index and… #1843

Merged
merged 6 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .zenodo.json
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@
"name": "Diez-Sierra, Javier",
"affiliation": "Santander Meteorology Group, Instituto de Física de Cantabria (CSIC-UC), Santander, Spain",
"orcid": "0000-0001-9053-2542"
},
{
"name": "Wang, Hui-Min",
"affiliation": "National University of Singapore, Singapore, Singapore",
"orcid": "0000-0002-5878-7542"
}
],
"keywords": [
Expand Down
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,4 @@ Contributors
* Dante Castro <[email protected]> `@profesorpaiche <https://github.com/profesorpaiche>`_
* Sascha Hofmann <[email protected]> `@saschahofmann <https://github.com/saschahofmann>`_
* Javier Diez-Sierra <[email protected]> `@JavierDiezSierra <https://github.com/JavierDiezSierra>`_
* Hui-Min Wang `@Hem-W <https://github.com/Hem-W>`
6 changes: 5 additions & 1 deletion CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ Changelog

v0.52.0 (unreleased)
--------------------
Contributors to this version: David Huard (:user:`huard`), Trevor James Smith (:user:`Zeitsperre`).
Contributors to this version: David Huard (:user:`huard`), Trevor James Smith (:user:`Zeitsperre`), Hui-Min Wang (:user:`Hem-W`).

Bug fixes
^^^^^^^^^
* Fixed the indexer bug in the `xclim.indices.standardized_index_fit_params` when multiple or non-array indexers are specified and fitted parameters are reloaded from netCDF. (:issue:`1842`, :pull:`1843`).

Internal changes
^^^^^^^^^^^^^^^^
Expand Down
28 changes: 24 additions & 4 deletions tests/test_indices.py
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,15 @@ def test_standardized_precipitation_evapotranspiration_index(

np.testing.assert_allclose(spei.values, values, rtol=0, atol=diff_tol)

def test_standardized_index_modularity(self, open_dataset):
@pytest.mark.parametrize(
"indexer",
[
({}),
({"month": [2, 3]}),
({"month": [2, 3], "drop": True}),
],
)
def test_standardized_index_modularity(self, open_dataset, tmp_path, indexer):
freq, window, dist, method = "MS", 6, "gamma", "APP"
ds = (
open_dataset("sdba/CanESM2_1950-2100.nc")
Expand All @@ -756,8 +764,15 @@ def test_standardized_index_modularity(self, open_dataset):
dist=dist,
method=method,
fitkwargs=fitkwargs,
month=[2, 3],
**indexer,
)

# Save the parameters to a file to test against that saving process may modify the netCDF file
paramsfile = tmp_path / "params0.nc"
params.to_netcdf(paramsfile)
params.close()
params = xr.open_dataset(paramsfile).__xarray_dataarray_variable__

spei1 = xci.standardized_precipitation_evapotranspiration_index(
wb.sel(time=slice("1998", "2000")), params=params
)
Expand All @@ -771,13 +786,18 @@ def test_standardized_index_modularity(self, open_dataset):
fitkwargs=fitkwargs,
cal_start="1950",
cal_end="1980",
month=[2, 3],
**indexer,
).sel(time=slice("1998", "2000"))

# In the previous computation, the first {window-1} values are NaN because the rolling is performed on the period [1998,2000].
# Here, the computation is performed on the period [1950,2000], *then* subsetted to [1998,2000], so it doesn't have NaNs
# for the first values
spei2[{"time": slice(0, window - 1)}] = np.nan
nan_window = xr.cftime_range(
spei1.time.values[0], periods=window - 1, freq=freq
)
spei2.loc[{"time": spei2.time.isin(nan_window)}] = (
np.nan
) # select time based on the window is necessary when `drop=True`

np.testing.assert_allclose(spei1.values, spei2.values, rtol=0, atol=1e-4)

Expand Down
6 changes: 3 additions & 3 deletions xclim/indices/stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import json
import warnings
from collections.abc import Sequence
from typing import Any
Expand Down Expand Up @@ -797,8 +798,7 @@ def standardized_index_fit_params(
"group": group,
"units": "",
}
method, args = ("", []) if indexer == {} else indexer.popitem()
params.attrs["time_indexer"] = (method, *args)
params.attrs["time_indexer"] = json.dumps(indexer)
if offset:
params.attrs["offset"] = offset
return params
Expand Down Expand Up @@ -879,7 +879,7 @@ def standardized_index(
)
# Unpack attrs to None and {} if needed
freq = None if freq == "" else freq
indexer = {} if indexer[0] == "" else {indexer[0]: indexer[1:]}
indexer = json.loads(indexer)
if cal_start or cal_end:
warnings.warn(
"Expected either `cal_{start|end}` or `params`, got both. The `params` input overrides other inputs."
Expand Down