CellMethods now printed in CF-compliant manner #4279

MoseleyS · 2021-08-11T09:43:15Z

🚀 Pull Request

Description

This simple PR rearranges the order in which CellMethod information is formatted by the str method. The grouping of this information (where multiple coords refer to the same method) has therefore been removed.

trexfeathers · 2021-08-11T09:51:51Z

Not being a CF expert, I'm not certain on the specifics here. However I am certain that it breaks the assumptions of the latest cube summary code, having just worked on that (#4206), so some of the doctests will likely fail at the very least.

Have a look at this commit for details. Perhaps you'll have a good idea how to adapt for CF-compliance: 4d8859c (specifically the changes in cube_summary.py).

trexfeathers · 2021-08-11T09:54:51Z

@MoseleyS you'll also need to be working from a much more up to date branch, since MoseleyS:3592_cellmethods_cfcompliant contains none of the latest CI checks. You can merge main into this branch, or rebase it.

MoseleyS · 2021-08-11T09:57:57Z

Looks like I forgot to update my fork first...

… grouping)

for more information, see https://pre-commit.ci

MoseleyS · 2021-08-11T10:17:51Z

This should work with the cube_summary changes as the format is changing from "{method}: {coord}" to "{coord}: {method}" (see #3592). I expect I will need to modify a few more unit tests though to accept the corrected ordering of information.

The block @trexfeathers pointed me to contains an oddity: content looks like it will always be identical to str(method) and ought to be refactored as such.

trexfeathers · 2021-08-11T10:44:15Z

@MoseleyS just to note: by all means don't treat the current setup as a straight jacket.

E.g: it may be that cube summary shouldn't be reading CellMethod's string representation at all, but rather working with specialised attributes.

Whatever seems best practice to you, in light of what would be truly CF-compliant.

- cube summary expects CellMethod.coord as `name` - Unit test for cube summary

…ew line chars.

…{method}"

for more information, see https://pre-commit.ci

trexfeathers

Thanks for slogging through this @MoseleyS, I think it's a great idea! I'd also appreciate an opinion from @pp-mo about the general concept here, since @pp-mo has more experience with CF.

I have a few comments for you. The main issue I have is the downstream parsing of CellMethod.__str__ in the cube summary code. I already struggled to de-risk this in my previous implementation, and now with your improved implementation that issue has become that extra bit problematic. See what you think of my suggestion.

Once all other changes are made, you will need to find existing examples in the documentation and change them to the new world before the doctests will pass. If it helps, all the failures are listed in the CI details (here for latest run at time of writing).

Finally: this needs a "What's New" entry.

trexfeathers · 2021-08-16T10:06:37Z

docs/src/userguide/interpolation_and_regridding.rst

-        Attributes:
+            time                        mean within years
+            time                        mean over years
+         Attributes:


Suggested change

Attributes:

Attributes:

trexfeathers · 2021-08-16T10:17:31Z

lib/iris/_representation/cube_summary.py

+            names = content.split(": ")[:-1]
+            # Remove "coord: " or "coord1: coord2: " from the front of the string, leaving the value.
+            value = content.split(": ")[-1]


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()

trexfeathers · 2021-08-16T10:45:34Z

lib/iris/coords.py


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


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

trexfeathers · 2021-08-16T10:59:12Z

lib/iris/coords.py

-        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)})"


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?

trexfeathers · 2021-08-16T11:37:21Z

lib/iris/coords.py

@@ -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):


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.

trexfeathers · 2021-08-16T11:44:52Z

lib/iris/_representation/cube_printout.py

+                    for title, value in zip(sect.names, sect.values):
+                        add_scalar_row(", ".join(title), value)


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.

trexfeathers · 2021-08-16T11:45:34Z

lib/iris/cube.py

-            Attributes:
+                time                        mean within years
+                time                        mean over years
+             Attributes:


Suggested change

Attributes:

Attributes:

MoseleyS · 2021-08-16T12:00:40Z

Thanks for the suggestions. I'm a bit busy with other work, so this is going to have to wait for a week or so, but in principle, I agree with your suggestions.

trexfeathers · 2022-10-05T09:36:19Z

@MoseleyS does this still matter to you? Can we get it over the line? Or should I close it?

Could perhaps bring to a UKMO 'Surgery'?

MoseleyS · 2022-10-05T09:53:26Z

@MoseleyS does this still matter to you? Can we get it over the line? Or should I close it?

Could perhaps bring to a UKMO 'Surgery'?

I'd forgotten about this. I'd still like it to happen as I like consistency. I'm not sure when I'm going to have time to look at this again though, perhaps next month. Feel free to take over if you'd like to.

trexfeathers · 2022-10-05T09:56:25Z

Thanks @MoseleyS. @pp-mo has pointed out there are arguments for the current implementation too, although we're not certain in either direction. I'll mark this for team discussion just to make sure everyone is happy.

trexfeathers · 2022-10-19T10:20:38Z

Summary from team discussion today (ping @pp-mo):

We still agree that the CellMethod.__str__() should print something CF compliant.

More complex for a Cube. The left hand text uses the names of objects wherever possible. This dimension is called foo, this coordinate is called bar, etcetera. That is where the 'controversy' lies - there is no clear answer for cell methods, since they do not have names but are instead provided as a sequence of operations that were performed on this data. So we'd like to go with a THIRD WAY 😁 :

    Cell methods:
        0                < my_cube.cell_methods[0].__str__() >
        1                < my_cube.cell_methods[1].__str__() >
        2                < my_cube.cell_methods[2].__str__() >

I still intend to pick this up, but likely not before December. @MoseleyS is free to pursue before then if they wish.

MoseleyS · 2022-10-19T13:11:44Z

    Cell methods:
        0                < my_cube.cell_methods[0].__str__() >
        1                < my_cube.cell_methods[1].__str__() >
        2                < my_cube.cell_methods[2].__str__() >
I still intend to pick this up, but likely not before December. @MoseleyS is free to pursue before then if they wish.

That's fine by me. Ideally, the CellMethod.__str__ method will return the CF-compliant format.
I doubt I'll get to this before December, but anything is possible I guess.

pp-mo · 2022-10-20T08:46:18Z

If everyone is saying "December", evidently this won't make it into Iris 3.4.
So I'll label it for 3.5

trexfeathers · 2023-04-04T08:29:46Z

Replaced by #5224

MoseleyS and others added 2 commits August 11, 2021 10:58

CellMethods now printed in CF-compliant manner (and therefore without…

4fa484f

… grouping)

[pre-commit.ci] auto fixes from pre-commit.com hooks

977a0f2

for more information, see https://pre-commit.ci

MoseleyS force-pushed the 3592_cellmethods_cfcompliant branch from e1f2eea to 977a0f2 Compare August 11, 2021 09:59

MoseleyS added 2 commits August 11, 2021 11:22

Corrects ordering of CellMethod string

230e7e9

Refactors content in CellMethodSection

d3a648f

MoseleyS and others added 13 commits August 11, 2021 11:46

bug fixes

1faed11

- cube summary expects CellMethod.coord as `name` - Unit test for cube summary

cube_summary now knows that str(CellMethod) can return strings with n…

add1eec

…ew line chars.

Updates doc builds to expect CellMethods in CF-compliant format

50e74b7

Updates expected result for new CellMethod representation

ccb526a

Corrects another unittest

a964e71

CellMethods applying to multiple coords are now "{coord1}: {coord2}: …

21f7507

…{method}"

CellMethods applying to both spatial coords are named with coord "area"

03f702e

[pre-commit.ci] auto fixes from pre-commit.com hooks

ec9429e

for more information, see https://pre-commit.ci

bug fix

10c31d8

bug fix to cube printout

7b19fd2

Removes extraneous space and corrects a couple of unittests

b0966e6

Removes attempt to convert x, y to area

de1ff64

Two other minor things that needed undoing

f93a490

MoseleyS marked this pull request as ready for review August 13, 2021 11:27

trexfeathers requested changes Aug 16, 2021

View reviewed changes

bjlittle assigned trexfeathers Sep 15, 2021

trexfeathers added the Status: Decision Required label Oct 5, 2022

trexfeathers removed the Status: Decision Required label Oct 19, 2022

ESadek-MO assigned HGWright Mar 23, 2023

HGWright mentioned this pull request Mar 31, 2023

Cf cell method #5224

Merged

trexfeathers closed this Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CellMethods now printed in CF-compliant manner #4279

CellMethods now printed in CF-compliant manner #4279

MoseleyS commented Aug 11, 2021 •

edited

Loading

trexfeathers commented Aug 11, 2021

trexfeathers commented Aug 11, 2021

MoseleyS commented Aug 11, 2021

MoseleyS commented Aug 11, 2021

trexfeathers commented Aug 11, 2021

trexfeathers left a comment •

edited

Loading

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

trexfeathers Aug 16, 2021

MoseleyS commented Aug 16, 2021

trexfeathers commented Oct 5, 2022

MoseleyS commented Oct 5, 2022

trexfeathers commented Oct 5, 2022

trexfeathers commented Oct 19, 2022

MoseleyS commented Oct 19, 2022

pp-mo commented Oct 20, 2022

trexfeathers commented Apr 4, 2023

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

		@@ -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):

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

CellMethods now printed in CF-compliant manner #4279

CellMethods now printed in CF-compliant manner #4279

Conversation

MoseleyS commented Aug 11, 2021 • edited Loading

🚀 Pull Request

Description

This simple PR rearranges the order in which CellMethod information is formatted by the str method. The grouping of this information (where multiple coords refer to the same method) has therefore been removed.

trexfeathers commented Aug 11, 2021

trexfeathers commented Aug 11, 2021

MoseleyS commented Aug 11, 2021

MoseleyS commented Aug 11, 2021

trexfeathers commented Aug 11, 2021

trexfeathers left a comment • edited Loading

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

trexfeathers Aug 16, 2021

Choose a reason for hiding this comment

MoseleyS commented Aug 16, 2021

trexfeathers commented Oct 5, 2022

MoseleyS commented Oct 5, 2022

trexfeathers commented Oct 5, 2022

trexfeathers commented Oct 19, 2022

MoseleyS commented Oct 19, 2022

pp-mo commented Oct 20, 2022

trexfeathers commented Apr 4, 2023

MoseleyS commented Aug 11, 2021 •

edited

Loading

trexfeathers left a comment •

edited

Loading