From b385dba4f103de70277e9a173c63e71a6d7cfae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Mon, 13 Jan 2025 16:13:25 -0500 Subject: [PATCH 1/8] warn fitkwargs & lmoments, shift gamma with floc --- CHANGELOG.rst | 8 ++++++++ src/xclim/indices/stats.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a4d922613..61da8f36e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,14 @@ Changelog ========= +v0.55.0 (unpublished) +-------------------- +Contributors to this version: Éric Dupuis (:user:`coxipi`). + +Internal changes +^^^^^^^^^^^^^^^^ +* There is now a warning stating that `fitkwargs` are not employed when using `lmoments3` distribution. One exception is the use of `floc` which is allowed with the gamma distribution. `floc` is used to shift the distribution before computing fitting parameters with the lmoments3 distribution since `loc=0` is always assumed in the library. + v0.54.0 (2024-12-16) -------------------- Contributors to this version: Trevor James Smith (:user:`Zeitsperre`), Pascal Bourgault (:user:`aulemahal`), Éric Dupuis (:user:`coxipi`), Sascha Hofmann (:user:`saschahofmann`). diff --git a/src/xclim/indices/stats.py b/src/xclim/indices/stats.py index 76a018baa..e42e6d770 100644 --- a/src/xclim/indices/stats.py +++ b/src/xclim/indices/stats.py @@ -59,6 +59,21 @@ def _fitfunc_1d(arr, *, dist, nparams, method, **fitkwargs): # lmoments3 will raise an error if only dist.numargs + 2 values are provided if len(x) <= dist.numargs + 2: return np.asarray([np.nan] * nparams) + if (type(dist).__name__ != "GammaGen" and len(fitkwargs.keys()) != 0) or ( + type(dist).__name__ == "GammaGen" + and set(fitkwargs.keys()) - {"floc"} != set() + ): + warnings.warn( + "Lmoments3 does not use `fitkwargs` arguments, except for `floc` with the Gamma distribution." + ) + if "floc" in fitkwargs and type(dist).__name__ == "GammaGen": + # lmoments3 assumes `loc` is 0, so `x` may need to be shifted + # note that `floc` must already be in appropriate units for `x` + params = dist.lmom_fit(x - fitkwargs["floc"]) + params["loc"] = fitkwargs["floc"] + params = list(params.values()) + else: + params = list(dist.lmom_fit(x).values()) params = list(dist.lmom_fit(x).values()) elif method == "APP": args, kwargs = _fit_start(x, dist.name, **fitkwargs) From 6209134b70838215d21d408fe294f18873fe2420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Mon, 13 Jan 2025 16:16:57 -0500 Subject: [PATCH 2/8] update CHANGELOG --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 61da8f36e..b4157122a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,7 +8,7 @@ Contributors to this version: Éric Dupuis (:user:`coxipi`). Internal changes ^^^^^^^^^^^^^^^^ -* There is now a warning stating that `fitkwargs` are not employed when using `lmoments3` distribution. One exception is the use of `floc` which is allowed with the gamma distribution. `floc` is used to shift the distribution before computing fitting parameters with the lmoments3 distribution since `loc=0` is always assumed in the library. +* There is now a warning stating that `fitkwargs` are not employed when using `lmoments3` distribution. One exception is the use of `floc` which is allowed with the gamma distribution. `floc` is used to shift the distribution before computing fitting parameters with the lmoments3 distribution since `loc=0` is always assumed in the library. (:issue:`2043`, :pull:`2045`). v0.54.0 (2024-12-16) -------------------- From 1fb25bd42f00ae9d987824f407733add4db6ea20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= <71575674+coxipi@users.noreply.github.com> Date: Tue, 14 Jan 2025 09:37:34 -0500 Subject: [PATCH 3/8] remove old line Co-authored-by: Pascal Bourgault --- src/xclim/indices/stats.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/xclim/indices/stats.py b/src/xclim/indices/stats.py index e42e6d770..897fa091b 100644 --- a/src/xclim/indices/stats.py +++ b/src/xclim/indices/stats.py @@ -74,7 +74,6 @@ def _fitfunc_1d(arr, *, dist, nparams, method, **fitkwargs): params = list(params.values()) else: params = list(dist.lmom_fit(x).values()) - params = list(dist.lmom_fit(x).values()) elif method == "APP": args, kwargs = _fit_start(x, dist.name, **fitkwargs) kwargs.setdefault("loc", 0) From 8eb2b51db385d8adc0e6aaa639586756ea9a4fd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Tue, 14 Jan 2025 09:46:43 -0500 Subject: [PATCH 4/8] fitkwargs&PWM=error, allow PWM in SPI/SPEI --- src/xclim/indices/_agro.py | 6 ++++-- src/xclim/indices/stats.py | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/xclim/indices/_agro.py b/src/xclim/indices/_agro.py index 52726c3f9..baa08662a 100644 --- a/src/xclim/indices/_agro.py +++ b/src/xclim/indices/_agro.py @@ -1150,6 +1150,7 @@ def standardized_precipitation_index( uses a deterministic function that does not involve any optimization. fitkwargs : dict, optional Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). + If method is `PWM`, `fitkwargs` should be empty, except for `floc` with `dist`=`gamma` which is allowed. cal_start : DateStr, optional Start date of the calibration period. A `DateStr` is expected, that is a `str` in format `"YYYY-MM-DD"`. Default option `None` means that the calibration period begins at the start of the input dataset. @@ -1281,11 +1282,12 @@ def standardized_precipitation_evapotranspiration_index( resampling, the window is an integer number of months. dist : {'gamma', 'fisk'} Name of the univariate distribution. (see :py:mod:`scipy.stats`). - method : {'APP', 'ML'} + method : {'APP', 'ML', 'PWM'} Name of the fitting method, such as `ML` (maximum likelihood), `APP` (approximate). The approximate method uses a deterministic function that doesn't involve any optimization. fitkwargs : dict, optional Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). + If method is `PWM`, `fitkwargs` should be empty, except for `floc` with `dist`=`gamma` which is allowed. cal_start : DateStr, optional Start date of the calibration period. A `DateStr` is expected, that is a `str` in format `"YYYY-MM-DD"`. Default option `None` means that the calibration period begins at the start of the input dataset. @@ -1311,7 +1313,7 @@ def standardized_precipitation_evapotranspiration_index( """ fitkwargs = fitkwargs or {} - dist_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} + dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP", "PWM"]} if dist in dist_methods: if method not in dist_methods[dist]: raise NotImplementedError( diff --git a/src/xclim/indices/stats.py b/src/xclim/indices/stats.py index 897fa091b..b021857e7 100644 --- a/src/xclim/indices/stats.py +++ b/src/xclim/indices/stats.py @@ -63,7 +63,7 @@ def _fitfunc_1d(arr, *, dist, nparams, method, **fitkwargs): type(dist).__name__ == "GammaGen" and set(fitkwargs.keys()) - {"floc"} != set() ): - warnings.warn( + raise ValueError( "Lmoments3 does not use `fitkwargs` arguments, except for `floc` with the Gamma distribution." ) if "floc" in fitkwargs and type(dist).__name__ == "GammaGen": @@ -884,8 +884,9 @@ def standardized_index( The approximate method uses a deterministic function that doesn't involve any optimization. zero_inflated : bool If True, the zeroes of `da` are treated separately. - fitkwargs : dict + fitkwargs : dict, optional Kwargs passed to ``xclim.indices.stats.fit`` used to impose values of certains parameters (`floc`, `fscale`). + If method is `PWM`, `fitkwargs` should be empty, except for `floc` with `dist`=`gamma` which is allowed. cal_start : DateStr, optional Start date of the calibration period. A `DateStr` is expected, that is a `str` in format `"YYYY-MM-DD"`. Default option `None` means that the calibration period begins at the start of the input dataset. From 2ef071c03d07ffc4670d427326a816c6340e4638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Tue, 14 Jan 2025 15:58:21 -0500 Subject: [PATCH 5/8] add tests --- src/xclim/indices/_agro.py | 6 +-- src/xclim/indices/stats.py | 2 +- tests/test_indices.py | 75 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/xclim/indices/_agro.py b/src/xclim/indices/_agro.py index baa08662a..fd9355959 100644 --- a/src/xclim/indices/_agro.py +++ b/src/xclim/indices/_agro.py @@ -1145,7 +1145,7 @@ def standardized_precipitation_index( i.e. a monthly resampling, the window is an integer number of months. dist : {"gamma", "fisk"} Name of the univariate distribution. (see :py:mod:`scipy.stats`). - method : {"APP", "ML"} + method : {"APP", "ML", "PWM"} Name of the fitting method, such as `ML` (maximum likelihood), `APP` (approximate). The approximate method uses a deterministic function that does not involve any optimization. fitkwargs : dict, optional @@ -1219,7 +1219,7 @@ def standardized_precipitation_index( >>> spi_3_fitted = standardized_precipitation_index(pr, params=params) """ fitkwargs = fitkwargs or {} - dist_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} + dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} if dist in dist_methods: if method not in dist_methods[dist]: raise NotImplementedError( @@ -1313,7 +1313,7 @@ def standardized_precipitation_evapotranspiration_index( """ fitkwargs = fitkwargs or {} - dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP", "PWM"]} + dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} if dist in dist_methods: if method not in dist_methods[dist]: raise NotImplementedError( diff --git a/src/xclim/indices/stats.py b/src/xclim/indices/stats.py index b021857e7..477f8d7cf 100644 --- a/src/xclim/indices/stats.py +++ b/src/xclim/indices/stats.py @@ -808,7 +808,7 @@ def standardized_index_fit_params( "Pass a value for `floc` in `fitkwargs`." ) - dist_and_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} + dist_and_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} dist = get_dist(dist) if dist.name not in dist_and_methods: raise NotImplementedError(f"The distribution `{dist.name}` is not supported.") diff --git a/tests/test_indices.py b/tests/test_indices.py index 22978b49d..c8444fec4 100644 --- a/tests/test_indices.py +++ b/tests/test_indices.py @@ -660,6 +660,38 @@ class TestStandardizedIndices: [-1.08627775, -0.46491398, -0.77806462, 0.31759127, 0.03794528], 2e-2, ), + ( + "D", + 1, + "gamma", + "PWM", + [-0.13002, 1.346689, 0.965731, 0.245408, -0.427896], + 2e-2, + ), + ( + "D", + 12, + "gamma", + "PWM", + [-0.209411, -0.086357, 0.636851, 1.022608, 0.634409], + 2e-2, + ), + ( + "MS", + 1, + "gamma", + "PWM", + [1.364243, 1.478565, 1.915559, -3.055828, 0.905304], + 2e-2, + ), + ( + "MS", + 12, + "gamma", + "PWM", + [0.577214, 1.522867, 1.634222, 0.967847, 0.689001], + 2e-2, + ), ], ) def test_standardized_precipitation_index( @@ -671,6 +703,13 @@ def test_standardized_precipitation_index( and Version(__numpy_version__) < Version("2.0.0") ): pytest.skip("Skipping SPI/ML/D on older numpy") + + # change `dist` to a lmoments3 object if needed + if method == "PWM": + lmom = pytest.importorskip("lmoments3.distr") + scipy2lmom = {"gamma": "gam"} + dist = getattr(lmom, scipy2lmom[dist]) + ds = open_dataset("sdba/CanESM2_1950-2100.nc").isel(location=1) if freq == "D": # to compare with ``climate_indices`` @@ -922,6 +961,42 @@ def test_zero_inflated(self, open_dataset): np.all(np.not_equal(spid[False].values, spid[True].values)), True ) + def test_PWM_and_fitkwargs(self, open_dataset): + pr = ( + open_dataset("sdba/CanESM2_1950-2100.nc") + .isel(location=1) + .sel(time=slice("1950", "1980")) + ).pr + + lmom = pytest.importorskip("lmoments3.distr") + # for now, only one function used + scipy2lmom = {"gamma": "gam"} + dist = getattr(lmom, scipy2lmom["gamma"]) + fitkwargs = {"floc": 0} + input_params = dict( + freq=None, + window=1, + method="PWM", + dist=dist, + fitkwargs=fitkwargs, + # doy_bounds=(180, 180), + ) + # this should not cause a problem + params_d0 = xci.stats.standardized_index_fit_params(pr, **input_params).isel( + dayofyear=0 + ) + np.testing.assert_allclose( + params_d0, np.array([5.63e-01, 0, 3.37e-05]), rtol=0, atol=2e-2 + ) + # this should cause a problem + fitkwargs["fscale"] = 1 + input_params["fitkwargs"] = fitkwargs + with pytest.raises( + ValueError, + match="Lmoments3 does not use `fitkwargs` arguments, except for `floc` with the Gamma distribution.", + ): + xci.stats.standardized_index_fit_params(pr, **input_params) + class TestDailyFreezeThawCycles: @pytest.mark.parametrize( From dbce9669227f78a032d53e09781197d9c7ebb091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Tue, 14 Jan 2025 16:05:20 -0500 Subject: [PATCH 6/8] update CHANGELOG --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e01f7af36..ea7a24d95 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,7 @@ New features and enhancements Internal changes ^^^^^^^^^^^^^^^^ * There is now a warning stating that `fitkwargs` are not employed when using `lmoments3` distribution. One exception is the use of `floc` which is allowed with the gamma distribution. `floc` is used to shift the distribution before computing fitting parameters with the lmoments3 distribution since `loc=0` is always assumed in the library. (:issue:`2043`, :pull:`2045`). +* `PWM` method (`lmoments3`) is now available to be used with the `gamma` distribution in ``xclim.indices.standardized_precipitation_index`` and ``xclim.indices.standardized_precipitation_evapotranspiration_index``. (:issue:`2043`, :pull:`2045`). v0.54.0 (2024-12-16) -------------------- From c5d02593ec46de3633d9b02cab97dd4c0dbdc955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Wed, 15 Jan 2025 13:18:00 -0500 Subject: [PATCH 7/8] remove conditions on PWM --- src/xclim/indices/_agro.py | 14 ++++++++------ src/xclim/indices/stats.py | 17 ++++++++++------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/xclim/indices/_agro.py b/src/xclim/indices/_agro.py index fd9355959..9637a1d50 100644 --- a/src/xclim/indices/_agro.py +++ b/src/xclim/indices/_agro.py @@ -1143,8 +1143,8 @@ def standardized_precipitation_index( window : int Averaging window length relative to the resampling frequency. For example, if `freq="MS"`, i.e. a monthly resampling, the window is an integer number of months. - dist : {"gamma", "fisk"} - Name of the univariate distribution. (see :py:mod:`scipy.stats`). + dist : {'gamma', 'fisk'} + Name of the univariate distribution. (see :py:mod:`scipy.stats`). All possible distributions are allowed with 'PWM'. method : {"APP", "ML", "PWM"} Name of the fitting method, such as `ML` (maximum likelihood), `APP` (approximate). The approximate method uses a deterministic function that does not involve any optimization. @@ -1225,7 +1225,8 @@ def standardized_precipitation_index( raise NotImplementedError( f"{method} method is not implemented for {dist} distribution." ) - else: + # Constraints on distributions except for PWM + elif method != "PWM": raise NotImplementedError(f"{dist} distribution is not yet implemented.") # Precipitation is expected to be zero-inflated @@ -1281,7 +1282,7 @@ def standardized_precipitation_evapotranspiration_index( Averaging window length relative to the resampling frequency. For example, if `freq="MS"`, i.e. a monthly resampling, the window is an integer number of months. dist : {'gamma', 'fisk'} - Name of the univariate distribution. (see :py:mod:`scipy.stats`). + Name of the univariate distribution. (see :py:mod:`scipy.stats`). All possible distributions are allowed with 'PWM'. method : {'APP', 'ML', 'PWM'} Name of the fitting method, such as `ML` (maximum likelihood), `APP` (approximate). The approximate method uses a deterministic function that doesn't involve any optimization. @@ -1313,13 +1314,14 @@ def standardized_precipitation_evapotranspiration_index( """ fitkwargs = fitkwargs or {} - dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} + dist_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} if dist in dist_methods: if method not in dist_methods[dist]: raise NotImplementedError( f"{method} method is not implemented for {dist} distribution" ) - else: + # Constraints on distributions except for PWM + elif method != "PWM": raise NotImplementedError(f"{dist} distribution is not yet implemented.") # Water budget is not expected to be zero-inflated diff --git a/src/xclim/indices/stats.py b/src/xclim/indices/stats.py index 477f8d7cf..e2c222ad5 100644 --- a/src/xclim/indices/stats.py +++ b/src/xclim/indices/stats.py @@ -808,14 +808,17 @@ def standardized_index_fit_params( "Pass a value for `floc` in `fitkwargs`." ) - dist_and_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} + dist_and_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} dist = get_dist(dist) - if dist.name not in dist_and_methods: - raise NotImplementedError(f"The distribution `{dist.name}` is not supported.") - if method not in dist_and_methods[dist.name]: - raise NotImplementedError( - f"The method `{method}` is not supported for distribution `{dist.name}`." - ) + if method != "PWM": + if dist.name not in dist_and_methods: + raise NotImplementedError( + f"The distribution `{dist.name}` is not supported." + ) + if method not in dist_and_methods[dist.name]: + raise NotImplementedError( + f"The method `{method}` is not supported for distribution `{dist.name}`." + ) da, group = preprocess_standardized_index(da, freq, window, **indexer) if zero_inflated: prob_of_zero = da.groupby(group).map( From 6052fe553c25254144545eef14a127d889cc55b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Dupuis?= Date: Mon, 20 Jan 2025 10:57:13 -0500 Subject: [PATCH 8/8] no PWM constraint for SPI either --- src/xclim/indices/_agro.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/xclim/indices/_agro.py b/src/xclim/indices/_agro.py index 9637a1d50..0601c3f0c 100644 --- a/src/xclim/indices/_agro.py +++ b/src/xclim/indices/_agro.py @@ -1219,11 +1219,12 @@ def standardized_precipitation_index( >>> spi_3_fitted = standardized_precipitation_index(pr, params=params) """ fitkwargs = fitkwargs or {} - dist_methods = {"gamma": ["ML", "APP", "PWM"], "fisk": ["ML", "APP"]} + + dist_methods = {"gamma": ["ML", "APP"], "fisk": ["ML", "APP"]} if dist in dist_methods: if method not in dist_methods[dist]: raise NotImplementedError( - f"{method} method is not implemented for {dist} distribution." + f"{method} method is not implemented for {dist} distribution" ) # Constraints on distributions except for PWM elif method != "PWM":