Added zonal (i.e. longitudinal) mean support to LBPROC save rule

[hdyson]

This is a fix to correctly set the LBPROC upon saving as a pp file for zonal mean data. I have not included a test - if you can point me in the direction of where the tests should be for a pp_save_rules fix, I'd be grateful.

For a bit of context, the closest test I could find was here However, that is acting on zonal mean being set in a particular cube attribute rather than the cube cell methods. Simplifying that test down to just time and longitudinal mean yields this:

cube = stock.lat_lon_cube()
cube.attributes["ukmo__process_flags"] = ['Time mean field', 'Zonal mean field']
temp_filename = iris.util.create_temp_filename(".pp")
iris.save(cube, temp_filename)
result = iris.fileformats.pp.load(temp_filename).next()
result.lbproc

Which correctly reports 192. However, this snippet using cell methods instead reports 128 (i.e. just time mean):

cube = stock.lat_lon_cube()
cube.cell_methods = (
iris.coords.CellMethod(method=u'mean', coords=u'time'),
iris.coords.CellMethod(method=u'mean', coords=u'longitude')
)
temp_filename = iris.util.create_temp_filename(".pp")
iris.save(cube, temp_filename)
result = iris.fileformats.pp.load(temp_filename).next()
result.lbproc

This PR corrects that.

[cpelley]

I would be inclined to create a new test class in iris/tests/integration/test_pp.py called TestSaveLBPROC.

I would also be inclined to write a new function in iris/fileformats/rules.py called is_zonal_mean perhaps.

pp_save_rules.txt change then perhaps become something like:

IF
    is_zonal_mean(cm)
THEN
    pp.lbproc += 128

This function can then handle latitude, longitude, grid_longitude and grid_latitude etc. Not handling these other cases might be seen as deceptive/unexpected behaviour to the end user.

Can an iris developer confirm what actions will be required to get this through a review?

Cheers

[marqh]

the handling of the pp_save_rules is planned to change in teh near future, so I would be inclined to keep the code in the save rules, and not add a helper function into the rules module

@pp-mo was working in this space, and may care to comment

i agree with @cpelley that a new test class in iris/tests/integration/test_pp.py would be sensible; we would certainly appreciate a test for the new behaviour

[hdyson]

I've added tests that cater to the specific cases we're interested in (time mean, zonal mean, time & zonal mean). There's two failing tests on an area of code that I don't think these changes touch - TimedOutExceptions on AreaWeightedRegrid tests i.e. similar issues to #1986. Any ideas?

[ajdawson]

There's two failing tests on an area of code that I don't think these changes touch - TimedOutExceptions on AreaWeightedRegrid tests i.e. similar issues to #1986. Any ideas?

Those can be safely ignored in this case. These tests fail intermittently due to timeouts, I believe @cpelley is on the case already so this shouldn't be a problem for too much longer.

[pp-mo]

Those can be safely ignored in this case.

To clarify, if you haven't seen this before:

• click on the red 'X' next to your commit in github, and then to sub-tests as required.
• re-run the ones which are timing out, by hitting the 'Restart Build' control (clockwise round-and-again arrow)
• wait to see if it happens again
• repeat until it obeys !

[ajdawson]

Don't you need to be a project dev to restart builds?

[pp-mo]

I would be inclined to keep the code in the save rules, and not add a helper function into the rules module

I'm agreed with that in this case.
I doubt that a specialist "is_zonal_mean" function makes the logic any much clearer, as the original is a one-line test anyway, and we're only likely to use it twice? As proposed, at least we see exactly what it is testing.
On the other hand, I do agree that both longitude and grid_longitude should be handled.

As for latitude/grid_latitude, I've not heard of that -- would you call it a "meridional mean" ? -- I don't think there is an LBPROC bit encoding for such a thing.

[pp-mo]

Don't you need to be a project dev to restart builds?

Sorry, I didn't know that.
I have re-spun it myself...

[hdyson]

On the other hand, I do agree that both longitude and grid_longitude should be handled.

I've added grid_longitude to the pp_save_rules - I have not added a separate test for this, it seems to me as though it's a bit redundant with the existing tests. If you'd prefer an additional test, I'll certainly add one.

Sorry, I didn't know that.
I have re-spun it myself...

If you could re-do your magic on the timing out tests, I'd be grateful.

[hdyson]

I hadn't noticed that there were conflicts again on this one - is the procedure that I keep fixing this each time there's a conflict, or do I wait and resolve the conflicts after it's reviewed?

Given how close the conflicting change is to this one, it's a shame the two fixes weren't combined.

[cpelley]

I hadn't noticed that there were conflicts again on this one - is the procedure that I keep fixing this each time there's a conflict, or do I wait and resolve the conflicts after it's reviewed?

Ordinarily rebasing first is preferable otherwise the reviewer is not reviewing the changes that are going onto trunk, however, before going to the effort of rebasing again - I'm happy to review this myself but were still going to need someone with the big green button to merge once I give it the thumbs up.

Any takers?

[hdyson]

If someone could wave a magic wand at the timing out test, I'd be grateful.

[bjlittle]

@hdyson Done 😉

[cpelley]

Formatting on this makes it difficult to read, instead:

    def create_cube(self):
        cube = Cube(np.zeros((2, 3, 4)))
        tunit = Unit('days since 0001-01-01 00:00:00', calendar='gregorian')
        tcoord = DimCoord(np.arange(2), standard_name='time', units=tunit)
        cube.add_dim_coord(tcoord, 0)
        ...

[hdyson]

There's another timing out test, so if someone could retry that test, that'd be great.

[bjlittle]

@hdyson
A minor point, but it's probably safer (from a maintenance point of view) to use the add_cell_method method ...

cm = CellMethod(method='mean', coords='time')
cube.add_cell_method(cm)

Note that, CellMethod is imported directly from iris.coords ... so there's no need to specify the full namespace i.e. iris.coords.CellMethod

If you agree, then rinse-and-repeat for all below ... ?

[hdyson]

Thanks @bjlittle - will do.

[bjlittle]

@hdyson I'm 👍 for this PR, once you've serviced some minor comments.

[bjlittle]

@hdyson
Just noticed this line (after a double-take) ... did you know that you can also do this Unit('days since epoch') ?

Which is kinda handy 😉

[hdyson]

@bjlittle Nope, that's a new one on me - but one I'll certainly recycle. Thanks!

[bjlittle]

@hdyson
Travis is failing due to PEP8 failures ...
test_pp.py:627:42: E127 continuation line over-indented for visual indent

[bjlittle]

@hdyson Push up your PEP8 fix and we'll get this merged 😉

[hdyson]

@bjlittle Apologies - I'd missed the '-a' when making the commit yesterday, so didn't even commit the fix, let alone push it.

Is something up with travis? It seems to be having problems running the tests.

[bjlittle]

@hdyson No worries. I'll try restarting Travis, although it seems to be a systemic problem (at their end?)

[bjlittle]

@hdyson Hmmm seems like some dependencies may have moved under our feet. Not your doing.

I'll need to investigate further, hang tight.

[bjlittle]

@hdyson I'll try to recreate the problem locally so that I can identify the dependency issue and generate a fix.

[bjlittle]

@hdyson Could you please rebase against upstream/master (see #2053) to include a fix that resolves the travis failures, thanks 😄

[hdyson]

@bjlittle Thanks, that solved it.

[bjlittle]

@hdyson Finally, a clean run through travis, phew!

I'm 👍 for this PR so could you please squash your commits into one and I'll happily merge!

[hdyson]

@bjlittle You may want to double check the squashed commit - it's not something I've done before. Is it worth perhaps adding the procedure to the iris development workflow instructions?

Thanks for all your help on this, everyone.

[bjlittle]

@hdyson Your squashed commit looks good to me.

Typically, if a PR contains many commits the reviewer can request that the author squashes their commits into one, as it makes the commit history on master that much less noisy. It's a normal practice, and a good skill to have, so congrats to you!

As of today, we've just changed the settings on the iris repo to take advantage of a new GitHub feature that allows the reviewer to automatically squash and merge multiple commits into one new merge commit. This saves the whole reviewer and author ding dong, requesting the author to squash. So, looking forward this shouldn't be much of an issue.

[ajdawson]

@hdyson - we have some documentaion on this in the developer's guide: http://scitools.org.uk/iris/docs/latest/developers_guide/gitwash/development_workflow.html#rewriting-commit-history. Even though we can squash on merge through the Github GUI, it is still worth understanding this aspect of git as it can be very helpful.

[bjlittle]

@hdyson Also, progit is a pretty handy free git resource.

[bjlittle]

@hdyson ... and we're over the line! Thanks!

[hdyson]

Thanks again everyone - much appreciated.

@ajdawson Sorry, my mistake: I didn't make the mental connection between rewriting commit history and squashing commits, so I had the right page, but not the right understanding. Consequently, I didn't read that section in sufficient depth to spot where it talks about combining commits.