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.

Sign up for GitHub

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 freq/offset/indexer attrs in SPI #1538

Merged
merged 12 commits into from
Dec 1, 2023
Merged

fix freq/offset/indexer attrs in SPI #1538

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
26b18a5
fix freq/offset/indexer attrs in SPI/SPEI
coxipi Nov 24, 2023
1858b4e
pull number
coxipi Nov 24, 2023
f6b3dcf
attrs for time_indexer only if non-empty
coxipi Nov 24, 2023
35fcb30
import necessary functions in example
coxipi Nov 26, 2023
78fef68
Merge branch 'master' into fix_spi_attrs
Zeitsperre Nov 29, 2023
061f03f
Merge branch 'master' into fix_spi_attrs
Zeitsperre Nov 30, 2023
70abdc8
Merge branch 'master' into fix_spi_attrs
Zeitsperre Nov 30, 2023
0ec03b4
store indexer as a tuple, and reuse in spi
coxipi Nov 30, 2023
7fcce92
Merge branch 'fix_spi_attrs' of https://github.com/Ouranosinc/xclim i…
coxipi Nov 30, 2023
36c08c6
correct attr name
coxipi Nov 30, 2023
0fc7687
use indexer in modularity test
coxipi Nov 30, 2023
68e3f88
update docstring to reflect new default values
coxipi Nov 30, 2023
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: 3 additions & 2 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ Changelog

v0.47.0 (unreleased)
--------------------
Contributors to this version: Juliette Lavoie (:user:`juliettelavoie`), Pascal Bourgault (:user:`aulemahal`), Trevor James Smith (:user:`Zeitsperre`).
Contributors to this version: Juliette Lavoie (:user:`juliettelavoie`), Pascal Bourgault (:user:`aulemahal`), Trevor James Smith (:user:`Zeitsperre`), Éric Dupuis (:user:`coxipi`)

New features and enhancements
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* New functions ``xclim.ensembles.robustness_fractions`` and ``xclim.ensembles.robustness_categories``. The former will replace ``xclim.ensembles.change_significance`` which is now deprecated and will be removed in xclim 0.49 (:pull:`1514`).

Bug fixes
^^^^^^^^^
* Fixed a bug with ``n_escore=-1`` in ``xclim.sdba.adjustment.NpdfTransform`` (:issue:`1515`, :pull:`1515`).
* Fixed a bug with ``n_escore=-1`` in ``xclim.sdba.adjustment.NpdfTransform`` (:issue:`1515`, :pull:`1516`).
* Fix wrong attributes in `xclim.indices.standardized_precipitation_index``, `xclim.indices.standardized_precipitation_evapotranspiration_index`` (:issue:`1537`, :pull:`1538`).

Internal changes
^^^^^^^^^^^^^^^^
Expand Down
5 changes: 3 additions & 2 deletions xclim/indices/_agro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1230,8 +1230,9 @@ def standardized_precipitation_index(
if paramsd != template.sizes:
params = params.broadcast_like(template)

spi = standardized_index(pr, params, **indexer)
spi = standardized_index(pr, params)
spi.attrs = params.attrs
spi.attrs["freq"] = freq or xarray.infer_freq(spi.time)
spi.attrs["units"] = ""
return spi

Expand Down Expand Up @@ -1324,7 +1325,7 @@ def standardized_precipitation_evapotranspiration_index(
"Proceeding with the value given in `params`."
)
offset = params_offset
offset = 0 if offset is None else convert_units_to(offset, wb, context="hydro")
offset = 0 if offset == "" else convert_units_to(offset, wb, context="hydro")
# Allowed distributions are constrained by the SPI function
if dist in ["gamma", "fisk"] and offset <= 0:
raise ValueError(
Expand Down
10 changes: 6 additions & 4 deletions xclim/indices/stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ def standardized_index_fit_params(
window: int,
dist: str,
method: str,
offset: Quantified | None = None,
offset: Quantified = "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Disclaimer: I don't really understand what the original issue is.

In indices/_conversion.py, there are a lot of instances of a Quantified parameter set to None. I suspect it would be best to have consistency in how we represent default null Quantified values.

Would doing something like
offset = offset or ""
let you keep None as the default but not mess with attribute records?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good way around, I'll adopt this instead, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original issue was what you describe, None and {} mess the attributes records, an array can'T be saved in netcdf. So I wanted to change the default values in the code (so that what is used in the code would be the same thing as in attrs), but as you suggest, simply changing was is stored in the attrs but keep using None in the rest of the code is also possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to do the same with other indices ?

**indexer,
) -> xr.DataArray:
r"""Standardized Index fitting parameters.
Expand All @@ -687,7 +687,7 @@ def standardized_index_fit_params(
uses a deterministic function that doesn't involve any optimization.
offset: Quantified
Distributions bounded by zero (e.g. "gamma", "fisk") can be used for datasets with negative values
by using an offset: `da + offset`.
by using an offset: `da + offset`. An empty string is interpreted as no offset.
\*\*indexer
Indexing parameters to compute the indicator on a temporal subset of the data.
It accepts the same arguments as :py:func:`xclim.indices.generic.select_time`.
Expand All @@ -712,7 +712,7 @@ def standardized_index_fit_params(
f"The method `{method}` is not supported for distribution `{dist}`."
)

if offset is not None:
if offset != "":
with xr.set_options(keep_attrs=True):
da = da + convert_units_to(offset, da, context="hydro")

Expand All @@ -729,10 +729,12 @@ def standardized_index_fit_params(
"scipy_dist": dist,
"method": method,
"group": group,
"time_indexer": indexer,
"units": "",
"offset": offset,
}
if indexer != {}:
params.attrs["time_indexer"] = str(indexer)

return params


Expand Down