Skip to content

Commit cc034ec

Browse files
davidhasselldavidhassell
committed
upstream merge
2 parents 4b1f7bd + 1668e0b commit cc034ec

File tree

4 files changed

+176
-58
lines changed

4 files changed

+176
-58
lines changed

‎Changelog.rst‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
version 3.16.0
2+
--------------
3+
4+
**2023-??-??**
5+
6+
* Fix bug that caused `cf.Field.collapse` to give incorrect results
7+
for the "sum", "sum_of_weights" and "sum_of_weights2" methods, only
8+
in the case that weights have been requested
9+
(https://github.com/NCAS-CMS/cf-python/issues/701)
10+
111
version 3.15.4
212
--------------
313

‎cf/field.py‎

Lines changed: 72 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@
163163
"average",
164164
"sd",
165165
"standard_deviation",
166+
"sum",
166167
"var",
167168
"variance",
168169
"sum",
@@ -3767,6 +3768,17 @@ def weights(
37673768
for key, w in comp.items():
37683769
comp[key] = Weights.scale(w, scale)
37693770

3771+
for w in comp.values():
3772+
if not measure:
3773+
w.override_units("1", inplace=True)
3774+
3775+
mn = w.minimum()
3776+
if mn <= 0:
3777+
raise ValueError(
3778+
"All weights must be positive. "
3779+
f"Got a weight of {mn}"
3780+
)
3781+
37703782
if components or methods:
37713783
# --------------------------------------------------------
37723784
# Return a dictionary of component weights, which may be
@@ -4952,7 +4964,7 @@ def collapse(
49524964
group_span=None,
49534965
group_contiguous=1,
49544966
measure=False,
4955-
scale=1,
4967+
scale=None,
49564968
radius="earth",
49574969
great_circle=False,
49584970
verbose=None,
@@ -5477,6 +5489,10 @@ def collapse(
54775489
.. note:: By default *weights* is `None`, resulting in
54785490
**unweighted calculations**.
54795491

5492+
.. note:: Unless the *method* is ``'integral'``, the
5493+
units of the weights are not combined with
5494+
the field's units in the collapsed field.
5495+
54805496
If the alternative form of providing the collapse method
54815497
and axes combined as a CF cell methods-like string via the
54825498
*method* parameter has been used, then the *axes*
@@ -5520,57 +5536,58 @@ def collapse(
55205536
time you could set ``weights=('area', 'T')``.
55215537

55225538
measure: `bool`, optional
5523-
Create weights which are cell measures, i.e. which
5524-
describe actual cell sizes (e.g. cell area) with
5525-
appropriate units (e.g. metres squared). By default the
5526-
weights are normalised and have arbitrary units.
5539+
If True, and *weights* is not `None`, create weights
5540+
which are cell measures, i.e. which describe actual
5541+
cell sizes (e.g. cell area) with appropriate units
5542+
(e.g. metres squared). By default the weights units
5543+
are ignored.
55275544

55285545
Cell measures can be created for any combination of
5529-
axes. For example, cell measures for a time axis are the
5530-
time span for each cell with canonical units of seconds;
5531-
cell measures for the combination of four axes
5532-
representing time and three dimensional space could have
5533-
canonical units of metres cubed seconds.
5546+
axes. For example, cell measures for a time axis are
5547+
the time span for each cell with canonical units of
5548+
seconds; cell measures for the combination of four
5549+
axes representing time and three dimensional space
5550+
could have canonical units of metres cubed seconds.
55345551

5535-
When collapsing with the ``'integral'`` method, *measure*
5536-
must be True, and the units of the weights are
5537-
incorporated into the units of the returned field
5552+
When collapsing with the ``'integral'`` method,
5553+
*measure* must be True, and the units of the weights
5554+
are incorporated into the units of the returned field
55385555
construct.
55395556

55405557
.. note:: Specifying cell volume weights via
55415558
``weights=['X', 'Y', 'Z']`` or
5542-
``weights=['area', 'Z']`` (or other equivalents)
5543-
will produce **an incorrect result if the
5544-
vertical dimension coordinates do not define the
5545-
actual height or depth thickness of every cell
5546-
in the domain**. In this case,
5547-
``weights='volume'`` should be used instead,
5548-
which requires the field construct to have a
5549-
"volume" cell measure construct.
5559+
``weights=['area', 'Z']`` (or other
5560+
equivalents) will produce **an incorrect
5561+
result if the vertical dimension coordinates
5562+
do not define the actual height or depth
5563+
thickness of every cell in the domain**. In
5564+
this case, ``weights='volume'`` should be
5565+
used instead, which requires the field
5566+
construct to have a "volume" cell measure
5567+
construct.
55505568

5551-
If ``weights=True`` then care also needs to be
5552-
taken, as a "volume" cell measure construct will
5553-
be used if present, otherwise the cell volumes
5554-
will be calculated using the size of the
5555-
vertical coordinate cells.
5569+
If ``weights=True`` then care also needs to
5570+
be taken, as a "volume" cell measure
5571+
construct will be used if present, otherwise
5572+
the cell volumes will be calculated using
5573+
the size of the vertical coordinate cells.
55565574

55575575
.. versionadded:: 3.0.2
55585576

55595577
scale: number or `None`, optional
55605578
If set to a positive number then scale the weights so
55615579
that they are less than or equal to that number. If
5562-
set to `None` the weights are not scaled. In general
5563-
the default is for weights to be scaled to lie between
5564-
0 and 1; however if *measure* is True then the weights
5565-
are never scaled and the value of *scale* is taken as
5566-
`None`, regardless of its setting.
5580+
set to `None`, the default, then the weights are not
5581+
scaled.
55675582

55685583
*Parameter example:*
55695584
To scale all weights so that they lie between 0 and
5570-
10 ``scale=10``.
5585+
1 ``scale=1``.
55715586

55725587
.. versionadded:: 3.0.2
55735588

5589+
.. versionchanged:: 3.16.0 Default changed to `None`
5590+
55745591
radius: optional
55755592
Specify the radius used for calculating the areas of
55765593
cells defined in spherical polar coordinates. The
@@ -6456,6 +6473,12 @@ def collapse(
64566473
# ------------------------------------------------------------
64576474
# Parse the methods and axes
64586475
# ------------------------------------------------------------
6476+
if measure and scale is not None:
6477+
raise ValueError(
6478+
"'scale' must be None when 'measure' is True. "
6479+
f"Got: scale={scale!r}"
6480+
)
6481+
64596482
if ":" in method:
64606483
# Convert a cell methods string (such as 'area: mean dim3:
64616484
# dim2: max T: minimum height: variance') to a CellMethod
@@ -6673,8 +6696,15 @@ def collapse(
66736696
g_weights = None
66746697
else:
66756698
if measure:
6676-
# Never scale weights that are cell measures
6677-
scale = None
6699+
if method not in (
6700+
"integral",
6701+
"sum_of_weights",
6702+
"sum_of_weights2",
6703+
):
6704+
raise ValueError(
6705+
f"Can't set measure=True for {method!r} "
6706+
"collapses"
6707+
)
66786708
elif method == "integral":
66796709
raise ValueError(
66806710
f"Must set measure=True for {method!r} collapses"
@@ -6770,8 +6800,14 @@ def collapse(
67706800
d_kwargs = {}
67716801
if weights is not None:
67726802
if measure:
6773-
# Never scale weights that are cell measures
6774-
scale = None
6803+
if method not in (
6804+
"integral",
6805+
"sum_of_weights",
6806+
"sum_of_weights2",
6807+
):
6808+
raise ValueError(
6809+
f"Can't set measure=True for {method!r} collapses"
6810+
)
67756811
elif method == "integral":
67766812
raise ValueError(
67776813
f"Must set measure=True for {method!r} collapses"

‎cf/test/test_Field.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,6 @@ def test_Field_collapse(self):
441441

442442
for axes in axes_combinations(f):
443443
for method in (
444-
"sum",
445444
"min",
446445
"max",
447446
"minimum_absolute_value",
@@ -451,8 +450,6 @@ def test_Field_collapse(self):
451450
"sample_size",
452451
"sum_of_squares",
453452
"median",
454-
"sum_of_weights",
455-
"sum_of_weights2",
456453
):
457454
for weights in (None, "area"):
458455
a = f.collapse(method, axes=axes, weights=weights).data
@@ -462,10 +459,13 @@ def test_Field_collapse(self):
462459
)
463460

464461
for method in (
462+
"sum",
465463
"mean",
466464
"mean_absolute_value",
467465
"mean_of_upper_decile",
468466
"root_mean_square",
467+
"sum_of_weights",
468+
"sum_of_weights2",
469469
):
470470
for weights in (None, "area"):
471471
if weights is not None:

‎cf/test/test_collapse.py‎

Lines changed: 91 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,6 @@ def setUp(self):
1616
os.path.dirname(os.path.abspath(__file__)), "test_file2.nc"
1717
)
1818

19-
self.test_only = []
20-
21-
# self.test_only = ['nought']
22-
# self.test_only = ['test_Field_collapse']
23-
# self.test_only = ['test_Field_collapse_CLIMATOLOGICAL_TIME']
24-
# self.test_only = ['test_Field_collapse_GROUPS']
25-
2619
def test_Field_collapse_CLIMATOLOGICAL_TIME(self):
2720
verbose = False
2821

@@ -373,18 +366,6 @@ def test_Field_collapse(self):
373366
f"{bound.day}!={group.offset.day}, group={group}",
374367
)
375368

376-
# for group in (cf.D(30),
377-
# cf.D(30, month=12),
378-
# cf.D(30, day=16),
379-
# cf.D(30, month=11, day=27)):
380-
# g = f.collapse('T: mean', group=group)
381-
# bound = g.coord('T').bounds.datetime_array[0, 1]
382-
# self.assertEqual(
383-
# bound.day, group.offset.day,
384-
# "{}!={}, bound={}, group={}".format(
385-
# bound.day, group.offset.day, bound, group)
386-
# )
387-
388369
def test_Field_collapse_WEIGHTS(self):
389370
verbose = False
390371

@@ -682,6 +663,97 @@ def test_Field_collapse_GROUPS(self):
682663
print(g.constructs)
683664
self.assertEqual(list(g.shape), expected_shape, g.shape)
684665

666+
def test_Field_collapse_sum(self):
667+
f = cf.example_field(0)
668+
w = f.weights("area", measure=True)
669+
a = f.array
670+
wa = w.array
671+
ws = a * wa
672+
673+
g = f.collapse("area: sum")
674+
self.assertTrue((g.array == a.sum()).all())
675+
676+
g = f.collapse("area: sum", weights=w)
677+
self.assertTrue((g.array == ws.sum()).all())
678+
self.assertEqual(g.Units, cf.Units("1"))
679+
680+
g = f.collapse("area: sum", weights=w, scale=1)
681+
self.assertTrue((g.array == (ws / wa.max()).sum()).all())
682+
self.assertEqual(g.Units, cf.Units("1"))
683+
684+
g = f.collapse("area: sum", weights=w)
685+
self.assertTrue((g.array == ws.sum()).all())
686+
self.assertEqual(g.Units, cf.Units("1"))
687+
688+
# Can't set measure=True for 'sum' collapses
689+
with self.assertRaises(ValueError):
690+
g = f.collapse("area: sum", weights=w, measure=True)
691+
692+
def test_Field_collapse_integral(self):
693+
f = cf.example_field(0)
694+
w = f.weights("area", measure=True)
695+
a = f.array
696+
wa = w.array
697+
ws = a * wa
698+
699+
g = f.collapse("area: integral", weights=w, measure=True)
700+
self.assertTrue((g.array == ws.sum()).all())
701+
self.assertEqual(g.Units, cf.Units("m2"))
702+
703+
# Must set the 'weights' parameter for 'integral' collapses
704+
with self.assertRaises(ValueError):
705+
g = f.collapse("area: integral")
706+
707+
# Must set measure=True for 'integral' collapses
708+
with self.assertRaises(ValueError):
709+
g = f.collapse("area: integral", weights=w)
710+
711+
# 'scale' must be None when 'measure' is True
712+
with self.assertRaises(ValueError):
713+
g = f.collapse("area: integral", weights=w, measure=True, scale=1)
714+
715+
def test_Field_collapse_sum_weights(self):
716+
f = cf.example_field(0)
717+
w = f.weights("area", measure=True)
718+
wa = w.array
719+
720+
g = f.collapse("area: sum_of_weights")
721+
self.assertTrue((g.array == 40).all())
722+
self.assertEqual(g.Units, cf.Units())
723+
724+
g = f.collapse("area: sum_of_weights", weights=w)
725+
self.assertTrue((g.array == wa.sum()).all())
726+
self.assertEqual(g.Units, cf.Units("1"))
727+
728+
g = f.collapse("area: sum_of_weights", weights=w, measure=True)
729+
self.assertTrue((g.array == wa.sum()).all())
730+
self.assertEqual(g.Units, cf.Units("m2"))
731+
732+
g = f.collapse("area: sum_of_weights", weights=w, scale=1)
733+
self.assertTrue((g.array == (wa / wa.max()).sum()).all())
734+
self.assertEqual(g.Units, cf.Units("1"))
735+
736+
def test_Field_collapse_sum_weights2(self):
737+
f = cf.example_field(0)
738+
w = f.weights("area", measure=True)
739+
wa = w.array**2
740+
741+
g = f.collapse("area: sum_of_weights2")
742+
self.assertTrue((g.array == 40).all())
743+
self.assertEqual(g.Units, cf.Units())
744+
745+
g = f.collapse("area: sum_of_weights2", weights=w)
746+
self.assertTrue((g.array == wa.sum()).all())
747+
self.assertEqual(g.Units, cf.Units("1"))
748+
749+
g = f.collapse("area: sum_of_weights2", weights=w, measure=True)
750+
self.assertTrue((g.array == wa.sum()).all())
751+
self.assertEqual(g.Units, cf.Units("m4"))
752+
753+
g = f.collapse("area: sum_of_weights2", weights=w, scale=1)
754+
self.assertTrue((g.array == (wa / wa.max()).sum()).all())
755+
self.assertEqual(g.Units, cf.Units("1"))
756+
685757

686758
if __name__ == "__main__":
687759
print("Run date:", datetime.datetime.now())

0 commit comments

Comments
 (0)