-
Notifications
You must be signed in to change notification settings - Fork 283
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.
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
TEST: Extends #2569 to unpickle #2608
Conversation
# Check that a GribMessage pickles without errors. | ||
messages = GribMessage.messages_from_filename(self.path) | ||
obj = next(messages) | ||
self.pickle_obj(obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpelley Is the test simply that there are no exceptions raised by pickle.dump()
within pickle_obj()
?
There are no explicit assert
s associated with this test nor test_data
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, these are the original two tests from this module which I refactored only to perform a common pickle dump (self.pickle_obj). I have added test_roundtrip
which performs a dump and load and performs an assert.
I do not know enough about grib to understand whether test
and test_data
actually test something that my new test_roundtrip
does not (i.e. redundant or not). Can you confirm whether these two tests still serve a purpose?
665875a
to
bfa9a76
Compare
Thanks @bjlittle. The test do use cPickle. Regarding the different protocols, I hadn't appreciated that the pickleability of an object could be sensitive to the protocol chosen -- I have added tests on all three protocols now (I would be interested to hear about here sensitivities might occur). |
Rebased :) |
Restarted the travis CI but sill unrelated failures and timeouts. |
@cpelley unfortunately not all of the test failures are unrelated to this change (although these test failures were right at the top of the list of failing tests). Sorry this is a little long:
At least it looks like it's the same thing going wrong in each case! Would you mind taking a look please... |
Hi @cpelley - thanks for this. I've just removed this from the v2.0.0 milestone and added the "bug" label. Essentially, I think we can merge this at any time (and I'm happy to do so), but do not consider this a release blocker. There will be a sweet-spot over the next 7 days where there is opportunity to merge simple PRs before the v2.0.0 release candidate - if you can update your PR in that time, I've no objection to it going into master (note: I haven't reviewed the actual code, so usual code review applies). |
Thanks @pelson not sure when I'll be able to do this. TBH, a fix to grib couldn't be any lower down on my priorities of things to do especially given my lack of exposure to it. Would you be open to me turning these into known failure tests for the grib so we can get the benefit of the unpickle test coverage for the fileformats? Cheers |
@cpelley Would you mind re-basing (again) ? |
bfa9a76
to
9f91552
Compare
After rebase, I get a strange exception:
Since I don't have an environment available that seems to have iris_gib in it. Changing the decorator order had no effect. If you have any ideas, feel free to jump in :) Cheers |
I looked into this. It only occurs whens tests are run with "nose", because its "discovery" methods are subtly different from unittest :-( I'm not clear why are you even using this - surely these tests ought to pass ?? |
Thanks for the comment @pp-mo
Gosh, that's unpleasant! Thanks for figuring this out.
This issue was intending to add the missing 'unpickle' part to the format testing ('pickle' testing for pp and netcdf being added in #2569). Cheers |
There are unrelated test failures in the python3 CI |
From what I can see, these are because the conda build has installed dask 2 (which is quite new, at least to iris). As usual, one more rebase fixes everything ... |
Yes, I think that's reasonable. It would be nice to get the grib-unpickle working too, at some point, but I guess we can wait until there is a real driver for that.
N.B. I also think it will be good practice to reference the PR in a comment (like the above). What do you think @cpelley ? |
I'm not sure why but replacing the failing ones with a decorated version in that one class effects the other test classes too! - puzzling Cheers |
Good stuff @cpelley But we have a problem ... no grib tests are now running (they are all skipped). Please wait on this, while we fix that. 😢 |
OK NP, thanks @pp-mo |
Hi @bjlittle |
No, I'm afraid this has definitely missed the cut for 2.3 😢 But the good news is, following action to drop python2, we should be re-enabling grib testing quite soon. |
OK, well I think this PR is good to go when your ready. |
It's coming. |
# see https://github.com/SciTools/iris/pull/2608 | ||
@unittest.expectedFailure | ||
def test_protocol_0(self): | ||
super(TestGribMessage, self).test_protocol_0() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpelley Now we're exclusively Python3, isn't this now simply super().test_protocol_0()
?
If true, also applies through out. Fix this and I'll merge 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TEST: Extends SciTools#2569 to unpickle
TEST: Extends SciTools#2569 to unpickle
TEST: Extends SciTools#2569 to unpickle
* Bump version to 2.4.0rc0. * unpin mpl (#3468) * Merge pull request #3301 from bayliffe/fastpercentilemethod_mask_test Analysis percentile method - update applicability test for fast_percentile_method * Have Travis test with iris-grib, remove problem tests (#3469) * Have Travis test with iris-grib, remove problem tests * mock GribInternalError correctly * Update license headers * account for changes in handling of grib message defaults * Test against the latest version of python-eccodes * Moved irir-grib skip to iris.tests * Merge pull request #2608 from cpelley/PICKLEABLE_FORMATS TEST: Extends #2569 to unpickle * _regrid_area_weighted_array: Move axes creation over which weights are calculated to before loop (#3519) * Purge iris.experimental.regrid np<1.7 support (#3539) * Add NameConstraint with relaxed name loading (#3463) * _regrid_area_weighted_array: move indices variable nearer to use (#3564) * _regrid_area_weighted_array: Tweak variable order to near other use in code (#3571) * Fix problems with export and echo command. (#3577) * Pushdocs fix2 (#3580) * Revert to single-line command for doctr invocation. * Added script comment, partly to force Github respin. * Bracketed six.moves and __future__ imports. * Fixes required due to the release of iris-grib v0.15.0 (#3582) * Fix python-eccodes pin in travis (#3593) * PI-2472: Optimise the area weighted regridding routine (#3598) * PI-2472: Tweak area weighting regrid move xdim and ydim axes (#3594) * _regrid_area_weighted_array: Set axis order to y_dim, x_dim last dimensions * _regrid_area_weighted_array: Extra tests for axes ordering * PI-2472: Tweak area weighting regrid enforce xdim ydim (#3595) * _regrid_area_weighted_array: Set axis order to y_dim, x_dim last dimensions * _regrid_area_weighted_array: Extra tests for axes ordering * _regrid_area_weighted_array: Ensure x_dim and y_dim * PI-2472: Tweak area weighting regrid move averaging out of loop (#3596) * _regrid_area_weighted_array: Refactor weights and move averaging outside loop * Pin pillow to make graphics tests work again. (#3630) * PI-2472: Area weighted regridding (#3623) * The Area-weights routine is refactored into the "__prepare" and "__perform" structure, in-line with our other regridding methods. * The area-weights are now computed in the "__prepare", so are calculated once. * The information required for checking the regridding weights and target grid info are now cached on the "regridder" object. * This is inline with the general use already described in the documentation. * pep8 conformance. * pep8 compliance. * Allow some 'key=None' args in Geostationary creation. (#3628) LGTM * Allow some 'key=None' args in Geostationary creation. * Integration test loading netcdf geostationary without offset properties. * pep8 conformance. * pep8 conformance. * pep8 conformance. * test_NameConstraint get mock from iris.tests. * Remove use of super() in _constraints.py for Py2 compatibility. * Updated license headers. * Updated iris-grib reference in extensions.txt. * Py2 support for iris-grib in Travis. * Updates for auto docs for Iris 2.4 release. * What's new entry to unpinning mpl. * Edited Py2 support for iris-grib in Travis. * Renamed whatsnew contributions folder for v2.4. * Hacked tests.integration.test_grib2 to avoid import error from iris-grib version < 0.15. * Only test grib with python 3. * Compiled v2.4 whatsnew. Co-authored-by: Martin Yeo <[email protected]> Co-authored-by: Bill Little <[email protected]> Co-authored-by: stephenworsley <[email protected]> Co-authored-by: abooton <[email protected]> Co-authored-by: lbdreyer <[email protected]> Co-authored-by: Emma Hogan <[email protected]>
Locally the grib roundtrip pickle-unpickle fails for me. I'll wait to see what happens on travis says before I post details.
Extends #2569 to check that we can unpickle.