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

CellMethods now printed in CF-compliant manner #4279

Closed
wants to merge 17 commits into from
Closed
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
10 changes: 5 additions & 5 deletions docs/src/userguide/interpolation_and_regridding.rst
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ Let's take the air temperature cube we've seen previously:
pressure 1000.0 hPa
time 1998-12-01 00:00:00, bound=(1994-12-01 00:00:00, 1998-12-01 00:00:00)
Cell methods:
mean within years time
mean over years time
time mean within years
time mean over years
Attributes:
STASH m01s16i203
source Data from Met Office Unified Model
Expand All @@ -94,9 +94,9 @@ We can interpolate specific values from the coordinates of the cube:
pressure 1000.0 hPa
time 1998-12-01 00:00:00, bound=(1994-12-01 00:00:00, 1998-12-01 00:00:00)
Cell methods:
mean within years time
mean over years time
Attributes:
time mean within years
time mean over years
Attributes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Attributes:
Attributes:

STASH m01s16i203
source Data from Met Office Unified Model

Expand Down
5 changes: 4 additions & 1 deletion lib/iris/_representation/cube_printout.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,12 @@ def add_scalar_row(name, value=""):
add_scalar_row(item.name, item.content)
if item.extra:
add_scalar_row(item_to_extra_indent + item.extra)
elif "attribute" in title or "cell method" in title:
elif "attribute" in title:
for title, value in zip(sect.names, sect.values):
add_scalar_row(title, value)
elif "cell method" in title:
for title, value in zip(sect.names, sect.values):
add_scalar_row(", ".join(title), value)
Comment on lines +264 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for title, value in zip(sect.names, sect.values):
add_scalar_row(", ".join(title), value)
for coords, method in zip(sect.names, sect.values):
add_scalar_row(", ".join(coords), method)

Specialised behaviour will benefit from specialised language to help understanding of what's going on.

elif "scalar cell measure" in title:
# These are just strings: nothing in the 'value' column.
for name in sect.contents:
Expand Down
10 changes: 5 additions & 5 deletions lib/iris/_representation/cube_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,12 @@ def __init__(self, title, cell_methods):
self.values = []
self.contents = []
for method in cell_methods:
name = method.method
# Remove "method: " from the front of the string, leaving the value.
value = str(method)[len(name + ": ") :]
self.names.append(name)
content = str(method)
names = content.split(": ")[:-1]
# Remove "coord: " or "coord1: coord2: " from the front of the string, leaving the value.
value = content.split(": ")[-1]
Comment on lines +226 to +228
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems risky to assume that : won't also appear in other places (especially if some of my other comments get actioned). Could you create CellMethod.method_str() for creating the 'right-hand-side' part of the string? Then the code could be:

names = method.coord_names
value = method.method_str()

self.names.append(names)
self.values.append(value)
content = "{}: {}".format(name, value)
self.contents.append(content)


Expand Down
19 changes: 9 additions & 10 deletions lib/iris/coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -2816,25 +2816,24 @@ def __init__(self, method, coords=None, intervals=None, comments=None):
self._init(method, tuple(_coords), tuple(_intervals), tuple(_comments))

def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in my cube_summary.py comment: could you abstract out the code that generates the method + interval + comment string (e.g. mean (6 minutes, This is a test comment, 12 minutes)) into its own method: self.method_str?

Then just have self.__str__ stitch together self.coord_names and self.method_str()? That would avoid the need for downstream string parsing, which can be risky and difficult to follow.

"""Return a custom string representation of CellMethod"""
"""Return a custom string representation of CellMethod in CF-format"""
# Group related coord names intervals and comments together
cell_components = zip_longest(
self.coord_names, self.intervals, self.comments, fillvalue=""
)

collection_summaries = []
cm_summary = "%s: " % self.method
coords = []
other_infos = []

for coord_name, interval, comment in cell_components:
coords.append(coord_name)
other_info = ", ".join(filter(None, chain((interval, comment))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at one of the examples, this currently generates:

6 minutes, This is a test comment

If we're aligning strictly to the conventions, should we actually be aiming for the following?

interval: 6 minutes comment: This is a test comment

if other_info:
coord_summary = "%s (%s)" % (coord_name, other_info)
else:
coord_summary = "%s" % coord_name

collection_summaries.append(coord_summary)
other_infos.append(other_info)

return cm_summary + ", ".join(collection_summaries)
result = f"{''.join([f'{c}: ' for c in coords])}{self.method}"
if any(other_infos):
result += f" ({', '.join(other_infos)})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result += f" ({', '.join(other_infos)})"
result += f" ({' '.join(other_infos)})"

If we're aligning strictly to the conventions, should we actually be separating with a space only?

return result

def __add__(self, other):
# Disable the default tuple behaviour of tuple concatenation
Expand Down
6 changes: 3 additions & 3 deletions lib/iris/cube.py
Original file line number Diff line number Diff line change
Expand Up @@ -750,9 +750,9 @@ class Cube(CFVariableMixin):
time \
1998-12-01 00:00:00, bound=(1994-12-01 00:00:00, 1998-12-01 00:00:00)
Cell methods:
mean within years time
mean over years time
Attributes:
time mean within years
time mean over years
Attributes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Attributes:
Attributes:

STASH m01s16i203
source Data from Met Office Unified Model

Expand Down
8 changes: 4 additions & 4 deletions lib/iris/tests/results/cdm/str_repr/cell_methods.__str__.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ air_temperature / (K) (latitude: 73; longitude: 96)
pressure 1000.0 hPa
time 1998-12-01 00:00:00
Cell methods:
mean longitude (6 minutes, This is a test comment), latitude (12 minutes)
average longitude (6 minutes, This is another test comment), latitude (15 minutes, This is another comment)
average longitude, latitude
percentile longitude (6 minutes, This is another test comment)
longitude, latitude mean (6 minutes, This is a test comment, 12 minutes)
longitude, latitude average (6 minutes, This is another test comment, 15 minutes, This is another comment)
longitude, latitude average
longitude percentile (6 minutes, This is another test comment)
Attributes:
STASH m01s16i203
source Data from Met Office Unified Model
10 changes: 5 additions & 5 deletions lib/iris/tests/unit/coords/test_CellMethod.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def setUp(self):
def _check(self, token, coord, default=False):
result = CellMethod(self.method, coords=coord)
token = token if not default else BaseMetadata.DEFAULT_NAME
expected = "{}: {}".format(self.method, token)
expected = "{}: {}".format(token, self.method)
self.assertEqual(str(result), expected)

def test_coord_standard_name(self):
Expand Down Expand Up @@ -64,27 +64,27 @@ def test_coord_stash_default(self):
def test_string(self):
token = "air_temperature"
result = CellMethod(self.method, coords=token)
expected = "{}: {}".format(self.method, token)
expected = "{}: {}".format(token, self.method)
self.assertEqual(str(result), expected)

def test_string_default(self):
token = "air temperature" # includes space
result = CellMethod(self.method, coords=token)
expected = "{}: unknown".format(self.method)
expected = "unknown: {}".format(self.method)
self.assertEqual(str(result), expected)

def test_mixture(self):
token = "air_temperature"
coord = AuxCoord(1, standard_name=token)
result = CellMethod(self.method, coords=[coord, token])
expected = "{}: {}, {}".format(self.method, token, token)
expected = "{0}: {0}: {1}".format(token, self.method)
self.assertEqual(str(result), expected)

def test_mixture_default(self):
token = "air temperature" # includes space
coord = AuxCoord(1, long_name=token)
result = CellMethod(self.method, coords=[coord, token])
expected = "{}: unknown, unknown".format(self.method)
expected = "unknown: unknown: {}".format(self.method)
self.assertEqual(str(result), expected)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,12 @@ def test_headings__cellmethods(self):
contents = self.representer.str_headings["Cell methods:"]
content_str = ",".join(content for content in contents)
for method in self.cube.cell_methods:
name = method.method
value = str(method)[len(name + ": ") :]
self.assertIn(name, content_str)
content = str(method)
names = content.split(":")[:-1]
# Remove "coord: " or "coord1: coord2: " from the front of the string, leaving the value.
value = content.split(":")[-1][1:]
for name in names:
self.assertIn(name, content_str)
self.assertIn(value, content_str)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -508,8 +508,8 @@ def test_section_cell_methods(self):
expected = [
"name / (1) (-- : 1)",
" Cell methods:",
" stdev area",
" mean y (10m, vertical), time (3min, =duration)",
" area stdev",
" y, time mean (10m, vertical, 3min, =duration)",
]
self.assertEqual(rep, expected)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ def test_cell_methods(self):

rep = CubeSummary(cube)
cell_method_section = rep.scalar_sections["Cell methods:"]
expected_contents = ["mean: x, y", "mean: x"]
expected_contents = ["x: y: mean", "x: mean"]
self.assertEqual(cell_method_section.contents, expected_contents)

def test_scalar_cube(self):
Expand Down