Fix lbfc mapping rules by pp-mo · Pull Request #965 · SciTools/iris

pp-mo · 2014-01-21T00:03:05Z

Addresses: #964

Fixes rule deriving lbfc from CF name+units; includes new "um_cf_map.py" from updated metOcean definitions; remove older rules this replaces.
Mostly wanted to simplify, but also because the existing rules do not check the units which is obviously dubious.

This requires metarelate/metOcean#7 to be merged first.

Prerequisites:

check metOcean PR completed Added name+units->lbfc definitions to replace Iris rules matching by name alone. metarelate/metOcean#7
recheck that new 'um_cf_map.py' matches latest metOcean content, via latest iris-code-generators

Reviewer: cpelley

pp-mo · 2014-01-22T14:19:32Z

Rebased to fix the Travis failure (cf. #968)

…Ocean definitions; remove older rules this replaces.

pp-mo · 2014-01-23T15:05:20Z

Rechecked + ready to go.

pp-mo · 2014-01-24T11:30:43Z

NOTE: I just deleted the branch by mistake, and then restored it. Hopefully this is ok..

cpelley · 2014-01-24T16:30:50Z

lib/iris/etc/pp_save_rules.txt

This wasn't a CF name. I realise this has not made it into the update to um_cf_map.py but do you know where wind_gust came from in the first place?

Any ideas @bjlittle, looks like it was your commit:
9728daa

Maybe not, @bjlittle only touched these rather than populating these:
#190
I still think we should understand where these came from.

I decided, following consulation with @marqh, that both this and the name "temperature" (above, line 77) are not sensible defiinitions and should simply be removed.
Conceivably this may cause someone problems, but I frankly doubt it, and the most practical way to find out is arguably just to do it + see if anyone complains.
Certainly, the possibility that this may cause someone a problem is not an argument for always leaving functionality as it is, if it is plainly "wrong". In principle, this is no different from our decision that these translations can no longer accept any-old units, which this change is also doing.

...are not sensible defiinitions and should simply be removed...

Agreed

Conceivably this may cause someone problems, but I frankly doubt it, and the most practical way to find out is arguably just to do it + see if anyone complains.

Agreed

cpelley · 2014-01-27T14:25:09Z

lib/iris/tests/unit/fileformats/pp/test_save.py

newcases only makes sense in the context of the here and now but I don't really that much

These look a lot like unit tests to me. Non-mockiphillic, of course.
Is there any reason they're not in the unit tests folder?

newcases only makes sense in the context of the here...

especially considering oldcases has been removed from this PR as being superfluous.

These are in the unit tests folder @bblay (lib/iris/tests/unit/fileformats/pp/test_save.py)

yes...you passed my test, well done....

cpelley · 2014-01-27T14:30:52Z

Looks good to me, rebase and I think its fine for going in (you might as well rename the unittest name as above at the same time).

@bblay you happy?

bblay · 2014-01-28T11:20:34Z

@bblay you happy?

I'm happy-go-lucky but I haven't looked at this PR, if that's what you mean.

bblay · 2014-02-03T13:31:00Z

bump

cpelley · 2014-02-04T13:31:07Z

I think that the unnecessary test_names_to_lbfc_newcases unittest can be removed, then its fine to be merged?

marqh · 2014-02-07T14:29:21Z

I am keen to see this merged and the follow up activity #989 also merged

bblay · 2014-02-07T15:35:57Z

I think that the unnecessary test_names_to_lbfc_newcases unittest can be removed

That test seems to have some small value, even though it only tests a small number of cases. It doesn't seem to be a unit test though, and I don't think the name is appropriate. I agree with @cpelley, please remove.

Note: I would hope that in the future the PP save rules will be written with carefully designed, functionally decomposed Python. In this case, the standard_name to lbfc conversion will probably be encapsulated into a single function suitable for a unit test.

cpelley · 2014-02-10T10:45:58Z

Back to you @pp-mo, minor change then it is ready

pp-mo · 2014-02-24T12:03:33Z

@cpelley Back to you @pp-mo

Thanks @cpelley : now addressed + I hope this is more sensible ?
Please re-review.

cpelley · 2014-02-25T09:41:38Z

Happy with the changes made, ready to merge.
Anyone like to take over from @bblay and merge this?

cpelley · 2014-02-26T11:14:40Z

Summary:

The removal of incorrect pp-CF mappings has been questioned from the perspective of changing existing behaviour. A reasoned argument has been accepted that the current behaviour was incorrect and for that reason alone should be removed.
Unittest coverage questioned in regard to having coverage of previous translations and all new translations. This has been resolved by the removal of unnecessary tests, in favour a sub sample of translations which test behaviour, with no intent on testing specific rules.

All issues raised have been addressed and the PR is ready.

cpelley · 2014-03-26T12:18:03Z

Bump, I think this PR is ready for merging if it has not become stale. Can you get someone assigned?

cpelley · 2014-04-23T07:53:17Z

Can we get someone to take press the green button on this? Another PR awaits this one (#989).

bjlittle · 2014-04-23T10:30:00Z

lib/iris/fileformats/um_cf_map.py

@pp-mo Have all of these changes been made to this file by hand?

I assume these were added as part of:
https://github.com/metarelate/metOcean/pull/7/files
(As defined in the prerequisites).

@pp-mo can you confirm this for us?

It's getting hard to remember (!)
I think this is a manual edit : The metarelate PR mentioned should match these, but I think there were concerns about how quickly that could be done (ha, ha), and how it might merge with other outstanding proposed changes there, so we proposed these changes separately and they have not come "via" metarelate in this case.
We were expecting these changes eventually to be subsumed by a new PR to install the latest metarelate output.

The changes themselves effectively "replace" the buggy rules removed from save_rules.txt (in sensible cases, that is).

cpelley · 2014-05-21T11:47:49Z

This PR is going quite again :(
From the discussion above, do you intend to make a PR with the https://github.com/metarelate/metOcean repository?

pp-mo · 2014-06-25T10:38:43Z

Superceded by #1155

pp-mo mentioned this pull request Jan 21, 2014

Added name+units->lbfc definitions to replace Iris rules matching by name alone. metarelate/metOcean#7

Merged

3 tasks

pp-mo added 2 commits January 23, 2014 12:53

Fix lbfc mapping from CF terms; include um_cf_map.py from updated met…

7e34e4f

…Ocean definitions; remove older rules this replaces.

Fixes for tests re-broken by rebase.

7d91507

pp-mo closed this Jan 24, 2014

pp-mo deleted the tidy_lbfc_rules branch January 24, 2014 11:27

pp-mo restored the tidy_lbfc_rules branch January 24, 2014 11:29

pp-mo reopened this Jan 24, 2014

cpelley reviewed Jan 24, 2014
View reviewed changes

Review action: remove superfluous test function.

2ddd98c

cpelley reviewed Jan 27, 2014
View reviewed changes

marqh mentioned this pull request Jan 28, 2014

moved stash translation #989

Closed

ghost assigned bblay Jan 28, 2014

Review action: tidy test methods.

e3caf8a

bjlittle reviewed Apr 23, 2014
View reviewed changes

pp-mo unassigned bblay Jun 3, 2014

marqh mentioned this pull request Jun 3, 2014

translation rework #1155

Merged

pp-mo closed this Jun 25, 2014

pp-mo deleted the tidy_lbfc_rules branch June 25, 2014 10:38

pp-mo restored the tidy_lbfc_rules branch June 25, 2014 10:38

pp-mo deleted the tidy_lbfc_rules branch March 14, 2016 13:51

Conversation

pp-mo commented Jan 21, 2014

Uh oh!

pp-mo commented Jan 22, 2014

Uh oh!

pp-mo commented Jan 23, 2014

Uh oh!

pp-mo commented Jan 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpelley commented Jan 27, 2014

Uh oh!

bblay commented Jan 28, 2014

Uh oh!

bblay commented Feb 3, 2014

Uh oh!

cpelley commented Feb 4, 2014

Uh oh!

marqh commented Feb 7, 2014

Uh oh!

bblay commented Feb 7, 2014

Uh oh!

cpelley commented Feb 10, 2014

Uh oh!

pp-mo commented Feb 24, 2014

Uh oh!

cpelley commented Feb 25, 2014

Uh oh!

cpelley commented Feb 26, 2014

Uh oh!

cpelley commented Mar 26, 2014

Uh oh!

cpelley commented Apr 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpelley commented May 21, 2014

Uh oh!

pp-mo commented Jun 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants