From 06f6badd8ac93dcff4883f3e578b2b12c468850c Mon Sep 17 00:00:00 2001 From: Pascal Bourgault Date: Thu, 15 Feb 2024 15:04:42 -0500 Subject: [PATCH 1/4] Sum is a sum - fix integral when orig has a temporal dim --- CHANGES.rst | 1 + tests/test_units.py | 26 ++++++++++++++++++++++++++ xclim/core/units.py | 18 ++++++++++++------ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 670bebdd4..104806c70 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -39,6 +39,7 @@ Breaking changes * `black` formatting style has been updated to the 2024 stable conventions. `isort` has been added to the `dev` installation recipe. (:pull:`1626`). * The indice and indicator for ``winter_storm`` has been removed (deprecated since `xclim` v0.46.0 in favour of ``snd_storm_days``). (:pull:`1565`). * `xclim` has dropped support for `scipy` version below v1.9.0 and `numpy` versions below v1.20.0. (:pull:`1565`). +* For generic function ``select_resample_op`` and ``core.units.to_agg_units``, operation "sum" will now return the same units as the input, and not implicitely translate to an "integral". (:issue:`1645`). Bug fixes ^^^^^^^^^ diff --git a/tests/test_units.py b/tests/test_units.py index 5dac52545..febcd27be 100644 --- a/tests/test_units.py +++ b/tests/test_units.py @@ -22,6 +22,7 @@ pint_multiply, rate2amount, str2pint, + to_agg_units, units, units2pint, ) @@ -317,3 +318,28 @@ def index(data: xr.DataArray, thresh: Quantified, dthreshdt: Quantified): with pytest.raises(ValidationError): index_full_mm("1 mm", "2 Pa", "3 mm/s") + + +@pytest.mark.parametrize( + "in_u,opfunc,op,exp,exp_u", + [ + ("m/h", "sum", "integral", 8760, "m"), + ("m/h", "sum", "sum", 365, "m/h"), + ("K", "mean", "mean", 1, "K"), + ("", "sum", "count", 365, "d"), + ("", "sum", "count", 365, "d"), + ("kg m-2", "var", "var", 0, "kg2 m-4"), + ("°C", "argmax", "doymax", 0, ""), + ], +) +def test_to_agg_units(in_u, opfunc, op, exp, exp_u): + da = xr.DataArray( + np.ones((365,)), + dims=("time",), + coords={"time": xr.cftime_range("1993-01-01", periods=365, freq="D")}, + attrs={"units": in_u}, + ) + + out = to_agg_units(getattr(da, opfunc)(), da, op) + np.testing.assert_allclose(out, exp) + assert out.attrs["units"] == exp_u diff --git a/xclim/core/units.py b/xclim/core/units.py index c6a73c91d..135099f3e 100644 --- a/xclim/core/units.py +++ b/xclim/core/units.py @@ -471,8 +471,8 @@ def to_agg_units( The original array before the aggregation operation, used to infer the sampling units and get the variable units. op : {'min', 'max', 'mean', 'std', 'var', 'doymin', 'doymax', 'count', 'integral', 'sum'} - The type of aggregation operation performed. The special "delta_*" ops are used - with temperature units needing conversion to their "delta" counterparts (e.g. degree days) + The type of aggregation operation performed. "integral" is mathematically equivalent to "sum", + but the units are multipled by the timestep of the data (requires an inferrable frequency). dim : str The time dimension along which the aggregation was performed. @@ -521,7 +521,7 @@ def to_agg_units( >>> degdays.units 'K d' """ - if op in ["amin", "min", "amax", "max", "mean", "std"]: + if op in ["amin", "min", "amax", "max", "mean", "std", "sum"]: out.attrs["units"] = orig.attrs["units"] elif op in ["var"]: @@ -532,7 +532,7 @@ def to_agg_units( units="", is_dayofyear=np.int32(1), calendar=get_calendar(orig) ) - elif op in ["count", "integral", "sum"]: + elif op in ["count", "integral"]: m, freq_u_raw = infer_sampling_units(orig[dim]) orig_u = str2pint(orig.units) freq_u = str2pint(freq_u_raw) @@ -540,8 +540,14 @@ def to_agg_units( if op == "count": out.attrs["units"] = freq_u_raw - elif op in ["integral", "sum"]: - out.attrs["units"] = pint2cfunits(orig_u * freq_u) + elif op == "integral": + if "[time]" in orig_u.dimensionality: + # We need to simplify units after multiplication + out_units = (orig_u * freq_u).to_reduced_units() + out = out * out_units.magnitude + out.attrs["units"] = pint2cfunits(out_units) + else: + out.attrs["units"] = pint2cfunits(orig_u * freq_u) else: raise ValueError( f"Unknown aggregation op {op}. " From 121a06b1339c53b4f3acdadc99979372bee27f28 Mon Sep 17 00:00:00 2001 From: Pascal Bourgault Date: Thu, 15 Feb 2024 15:11:14 -0500 Subject: [PATCH 2/4] Upd chgs --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 104806c70..c8e81c5d4 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -39,7 +39,7 @@ Breaking changes * `black` formatting style has been updated to the 2024 stable conventions. `isort` has been added to the `dev` installation recipe. (:pull:`1626`). * The indice and indicator for ``winter_storm`` has been removed (deprecated since `xclim` v0.46.0 in favour of ``snd_storm_days``). (:pull:`1565`). * `xclim` has dropped support for `scipy` version below v1.9.0 and `numpy` versions below v1.20.0. (:pull:`1565`). -* For generic function ``select_resample_op`` and ``core.units.to_agg_units``, operation "sum" will now return the same units as the input, and not implicitely translate to an "integral". (:issue:`1645`). +* For generic function ``select_resample_op`` and ``core.units.to_agg_units``, operation "sum" will now return the same units as the input, and not implicitely translate to an "integral". (:issue:`1645`, :pull:`1649`). Bug fixes ^^^^^^^^^ From ea148d94a57939a1b576781be40303b8b6ce074e Mon Sep 17 00:00:00 2001 From: Pascal Bourgault Date: Thu, 15 Feb 2024 15:33:36 -0500 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com> --- CHANGES.rst | 2 +- xclim/core/units.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index c8e81c5d4..cfa1aca15 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -39,7 +39,7 @@ Breaking changes * `black` formatting style has been updated to the 2024 stable conventions. `isort` has been added to the `dev` installation recipe. (:pull:`1626`). * The indice and indicator for ``winter_storm`` has been removed (deprecated since `xclim` v0.46.0 in favour of ``snd_storm_days``). (:pull:`1565`). * `xclim` has dropped support for `scipy` version below v1.9.0 and `numpy` versions below v1.20.0. (:pull:`1565`). -* For generic function ``select_resample_op`` and ``core.units.to_agg_units``, operation "sum" will now return the same units as the input, and not implicitely translate to an "integral". (:issue:`1645`, :pull:`1649`). +* For generic function ``select_resample_op`` and ``core.units.to_agg_units``, operation "sum" will now return the same units as the input, and not implicitly be translated to an "integral". (:issue:`1645`, :pull:`1649`). Bug fixes ^^^^^^^^^ diff --git a/xclim/core/units.py b/xclim/core/units.py index 135099f3e..2d58a33f8 100644 --- a/xclim/core/units.py +++ b/xclim/core/units.py @@ -472,7 +472,7 @@ def to_agg_units( used to infer the sampling units and get the variable units. op : {'min', 'max', 'mean', 'std', 'var', 'doymin', 'doymax', 'count', 'integral', 'sum'} The type of aggregation operation performed. "integral" is mathematically equivalent to "sum", - but the units are multipled by the timestep of the data (requires an inferrable frequency). + but the units are multiplied by the timestep of the data (requires an inferrable frequency). dim : str The time dimension along which the aggregation was performed. From 2fba9241a2dc3d9fa1d0274af39cea18cb4dbed7 Mon Sep 17 00:00:00 2001 From: Pascal Bourgault Date: Fri, 16 Feb 2024 10:34:40 -0500 Subject: [PATCH 4/4] Fix misusage of "sum" --- tests/test_atmos.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_atmos.py b/tests/test_atmos.py index 214e0dbfc..1d5a6e6fc 100644 --- a/tests/test_atmos.py +++ b/tests/test_atmos.py @@ -280,7 +280,7 @@ def test_wind_power_potential_from_3h_series(): power = out * 100 power.attrs["units"] = "MW" annual_power = convert_units_to( - select_resample_op(power, op="sum", freq="D"), "MWh" + select_resample_op(power, op="integral", freq="D"), "MWh" ) assert (annual_power == 100 * 24).all()