Skip to content

Conversation

@corinnebosley
Copy link
Member

Round-trip integration tests for hybrid height and hybrid pressure.

Also a small fix for a bit that was forgotten and only revealed in these tests.

@corinnebosley
Copy link
Member Author

@dkillick @pp-mo Should I put in some bits at the end of each test to delete the saved files?

def setUp(self):
self.filepath = self.get_testdata_path(
'faked_sample_hh_grib_data.grib2')
self.out_path = self.get_result_path(
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't store these results here (or even create this branch of the repo), as these are transient results.
Files living in tests/results should really be "independent" data that we want to store in the repo as a reference to compare results against.
Instead, I think it's appropriate to create temporary files which are then removed afterwards, using this pattern ...


class TestHybridHeightRoundTrip(tests.IrisGribTest):
def setUp(self):
self.filepath = self.get_testdata_path(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is actually worth a separate "setUp".
Depends if this will get shared with any other possible tests (later, presumably).

"""
gribapi.grib_set(grib, "productDefinitionTemplateNumber", 1)
product_definition_template_common(cube, grib)
product_definition_template_common(cube, grib, full3d_cube)
Copy link
Member

Choose a reason for hiding this comment

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

Whoops ?!?
Another reason to have more integration tests, if this could get missed.

@pp-mo
Copy link
Member

pp-mo commented Jun 12, 2018

"Otherwise" LGTM 👍 -- short, but if it works that's already proving quite a lot.
Sorry, meant to create a proper review but pressed wrong button.

@corinnebosley
Copy link
Member Author

@pp-mo Thanks for that, useful pointers, especially the temp_filepath pattern that I didn't know about before. Cool.

@corinnebosley
Copy link
Member Author

Wait a minute though, I still need to delete the save directories that I had before...

@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+0.05%) to 85.48% when pulling 1c9dcf8 on corinnebosley:hybrid_coords_round_trip into be1bc83 on SciTools:master.

@corinnebosley
Copy link
Member Author

@pp-mo @dkillick @kaedonkers Tests now passing. Any more tweaks?

@pp-mo
Copy link
Member

pp-mo commented Jun 12, 2018

Any more tweaks?

Maybe just a comment to say that/why it is loading+save a separate reference cube.

I'm also interested in your save(... saver='grib2') usage as I didn't know it could do that !
The docstrings as-is suggest it can only be a saver function.
Obviously that's an Iris issue... (--> SciTools/iris#3062)

@corinnebosley
Copy link
Member Author

@pp-mo Yeah I didn't know you could do that either, it was a bit of guesswork that got lucky because the docs are really really vague.

@pp-mo pp-mo merged commit bd477d2 into SciTools:master Jun 12, 2018
@corinnebosley
Copy link
Member Author

@pp-mo Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants