Skip to content

Granularity for fixed surface arguments #146

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.

Sign up for GitHub

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

Jump to bottom
Merged
bjlittle merged 9 commits into from
Jul 23, 2019
Merged

Granularity for fixed surface arguments #146

bjlittle merged 9 commits into from
Jul 23, 2019

Conversation

@trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Jun 4, 2019 •
edited
Loading

product_definition_section will no longer always try to populate the fixed surface arguments for translate_phenomenon - not available in some templates, in which case populate with None

Ran iris-grib unit tests afterwards - 364 passed, 21 were ignored as they needed external data (total 385)

product_definition_section will no longer try to populate the fixed s…
b725be8 
…urface arguments for translate_phenomenon - not available in some templates, in which case populate with None
@bjlittle
Copy link
Member

bjlittle commented Jun 5, 2019

Hey @trexfeathers!

Thanks for the PR 😄

You'll need to pep8 the iris_grib._load_convert.py file to detect non-compliance's - by eye there is least one line too long, and there perhaps might be some formatting issues.

Also, you need to change the file header from:

# (C) British Crown Copyright 2014 - 2018, Met Office
to
# (C) British Crown Copyright 2014 - 2019, Met Office

Also, you'll need to sign the CLA in order for us to accept your PR contribution

@coveralls
Copy link

coveralls commented Jun 5, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 86.425% when pulling 697c273 on trexfeathers:ff_surface_granularity into 8344a57 on SciTools:master.

corinnebosley
corinnebosley previously requested changes Jun 5, 2019
View reviewed changes
Copy link
Member

@corinnebosley corinnebosley left a comment

Choose a reason for hiding this comment

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

Nice changes @trexfeathers, and it passes all the Travis tests, which is awesome!

There is one more thing which I would like in this PR though, which is one test to make sure that loading doesn't fail when there is no variable representing typeOfFirstFixedSurface.

Maybe this could go into 'iris_grib/tests/unit/load_convert/test_translate_phenomenon.py', or alternatively you could create a new test file called 'iris_grib/tests/unit/load_convert/test_product_definition_section.py' and create some tests there instead. Your choice really.

@bjlittle bjlittle added this to the v0.15.x milestone Jun 5, 2019
trexfeathers added 4 commits June 5, 2019 17:18
Creating test_product_definition_section.py
96955ad
Finished test_product_definition_section.py (needs commenting)
0ecc2b8 
Made section_4 importable from test_product_definition_template_31.py
Finished commenting test_product_definition_section.py and test_produ…
9c05b27 
…ct_definition_template_31.py
Corrected pep8 non-conformance in test_product_definition_section.py
4e23daf 
Corrected license header in test_product_definition_template_31.py
@trexfeathers
Copy link
Contributor Author

trexfeathers commented Jun 7, 2019 •
edited
Loading

There is one more thing which I would like in this PR though, which is one test to make sure that loading doesn't fail when there is no variable representing typeOfFirstFixedSurface.

@corinnebosley if the template should contain the fixed surface keys, but does not, one would still want an error to occur. It seemed best to create 4 tests for all combinations of the fixed surface keys being expected/not and present/not, so that is what I have done 👌

bjlittle
bjlittle reviewed Jul 11, 2019
View reviewed changes
@bjlittle bjlittle self-assigned this Jul 11, 2019
bjlittle
bjlittle requested changes Jul 11, 2019
View reviewed changes
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@trexfeathers Okay, I'm reviewed out now...

Over to you 😄

trexfeathers added 2 commits July 17, 2019 12:57
Modifications following bjlittle review
f6ee794 
test_all currently fails. Could do with outputting more info on which combination is failing
Adapted call_count use since test class is now preserved - call_count…
fff6ad9 
… increases instead of resetting

Added print output of each test combination since they are all run as one
@trexfeathers
Copy link
Contributor Author

trexfeathers commented Jul 18, 2019

@bjlittle thanks for the review, it feels neater now. See what you think 😰

@bjlittle
Copy link
Member

bjlittle commented Jul 23, 2019

@trexfeathers I've requested only one final minor change, otherwise LGTM! 👍

Removed print output of each test combination - could be misleading i…
697c273 
…f running in parallel so best have the single test as a bit of a black box
bjlittle
bjlittle approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

Awesome PR, thanks @trexfeathers 👍

@bjlittle bjlittle dismissed corinnebosley’s stale review July 23, 2019 14:42

Issues resolved

@bjlittle bjlittle merged commit 00a72eb into SciTools:master Jul 23, 2019
@stephenworsley stephenworsley mentioned this pull request Oct 16, 2019
Merged
@lbdreyer lbdreyer mentioned this pull request Nov 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjlittle bjlittle bjlittle approved these changes

@corinnebosley corinnebosley corinnebosley left review comments

Assignees

@bjlittle bjlittle

Labels

SemVer: Minor Type: Bug

Projects

None yet

Milestone

v0.15

Development

Successfully merging this pull request may close these issues.

5 participants

@trexfeathers @bjlittle @coveralls @corinnebosley @SciTools-assistant