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

Sum is a sum #1649

Merged
merged 7 commits into from
Feb 16, 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
1 change: 1 addition & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,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 implicitly be translated to an "integral". (:issue:`1645`, :pull:`1649`).

Bug fixes
^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion tests/test_atmos.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
26 changes: 26 additions & 0 deletions tests/test_units.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
pint_multiply,
rate2amount,
str2pint,
to_agg_units,
units,
units2pint,
)
Expand Down Expand Up @@ -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
18 changes: 12 additions & 6 deletions xclim/core/units.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 multiplied by the timestep of the data (requires an inferrable frequency).
dim : str
The time dimension along which the aggregation was performed.

Expand Down Expand Up @@ -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"]:
Expand All @@ -532,16 +532,22 @@ 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)
out = out * m

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}. "
Expand Down