Skip to content

Conversation

@lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented Feb 23, 2022

The 0.17.x main branch has become quite stale. The lockfile that defines the CI environment had not been updated so was still using older versions of packages. I have since updated the lockfile and noticed a few test failures that this PR addresses.

I have split the commits so they each address a separate point:

  • d3c5112
    This commit addresses lockfile related updates:

    • Updates the lockfile to pull in latest versions of packages (as run this morning 2022/03/04)
    • Moves the lockfiles directory location from requirements/nox.lock to requirements/ci/nox.lock
    • Drops Py37 as this was dropped by Iris 3.2
  • Adds pip as an optional dependency (as we do for iris)

  • a389ed9
    Since cftime 1.4, cftime.date2num returns integers rather than floats so some cml needed updating:

>>> cftime.__version__
'1.3.1'
>>> date = datetime(2013, 2, 1)
>>> unit = 'hours since 1970-01-01 00:00:00'
>>> calendar = 'gregorian'
>>> cftime.date2num(date, unit, calendar)
377688.0

>>> cftime.__version__ 
'1.4.0'
>>> date = datetime(2013, 2, 1)
>>> unit = 'hours since 1970-01-01 00:00:00'
>>> calendar = 'gregorian'
>>> cftime.date2num(date, unit, calendar)
377688
    gribapi.grib_set_long(grib, "scaledValueOfRadiusOfSphericalEarth", -1)

I have replaced these all to be either GRIB_MISSING_LONG for longs or 255 for bytes.

  • 7ff42d1
    Since cartopy 0.20 and proj 8.0.1, ccrs.Mercator.transform_point() returns very slightly different values!. This is causing problems in a test for grid_definition_template_10 in which we compare what iris_grib._load_convert.grid_definition_template_10 returns against expected values. As that function calls ccrs.Mercator.transform_point(), the 'projection_y_coordinate' now has slightly different values to the expected values. As our coord comparison is so strict (expects exact match for coordinate points) the test fails. I have update the expected number so that it now matches.

  • 27a0f77
    Since Iris 3.2, loading NAME files that we use as part of iris-grib testing has slightly changed. The tests for NameII and NameII test loading in the NAME file, saving it out to GRIB then the loading it back in to check that is keeps the metadata we want to keep. The initial loading of the NAME file has changed, in particular:

    • there is an extra attribute `{'positive' : 'up' } that doesn't get saved out to GRIB (I don't think we should expect it to) so when we load the GRIB file back in and compare it to the original cube, it complains about the missing attribute. I've updated our load_callback in the test to now also add the attribute at load time (as we do with other missing metadata we don't care about).
    • the NAME cubes which contain a z coordinate called 'z', now are being identified as a coordinate that fits 'iris.coords(axis='z'), whereas previously iris.coords(axis='z') would return nothing. This is a problem when we try to save out to grib. Previously, when checking for vertical coordinates iris.coords(axis='z') would return nothing so it would just ignore the 'z' coordinate and it wouldn't get saved out to GRIB. Now, when checking for vertical coordinates, iris.coords(axis='z') returns the z coordinate so it is failing as it doesn't know what to do with it. To solve this I just removed that coordinate.
      Note this also includes some CML updates related to the cftime issue mentioned earlier

@lbdreyer lbdreyer marked this pull request as draft February 23, 2022 15:03
@lbdreyer
Copy link
Member Author

This is currently pointing at the v0.17.x branch, but we are going to do the mergeback of v0.17.x onto main first (#289). Once that is in, I will rebase onto main and instead point this at main instead.

I will put it into draft until then to ensure this doesn't accidentally get merged

@pp-mo
Copy link
Member

pp-mo commented Mar 4, 2022

Hi @lbdreyer do you think this casts light on #277 -- whether we should just drop testing against latest-iris ?
If we have effectively concluded that testing against iris dev-version is worth the effort, then maybe #277 should be retired.

@lbdreyer lbdreyer changed the base branch from v0.17.x to main March 4, 2022 12:05
@lbdreyer lbdreyer marked this pull request as ready for review March 4, 2022 12:09
@lbdreyer
Copy link
Member Author

lbdreyer commented Mar 4, 2022

This PR now bases of the main branch. I updated the first commit with a new, refreshed lockfile.

@lbdreyer
Copy link
Member Author

lbdreyer commented Mar 4, 2022

Hi @lbdreyer do you think this casts light on #277 -- whether we should just drop testing against latest-iris ?
If we have effectively concluded that testing against iris dev-version is worth the effort, then maybe #277 should be retired.

I'm in two minds about this. Is there much value in catching breaking Iris changes early if it means we are constantly breaking iris-grib's CI. Or could we rather just manually test against latest-iris as the need arises.
I'd like a little more time to think about this. For now though, I think we can keep testing against main

@lbdreyer
Copy link
Member Author

lbdreyer commented Mar 4, 2022

Coincidentally eccodes just released version 2.25 although it's not yet on conda-forge. That being said I'd still like to get this PR in then we can address any potential issues with 2.25 next week/when it is available

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Can't see anything wrong, but a couple of things where I thought it might be worth considering an alternative. See what you think

@lbdreyer
Copy link
Member Author

lbdreyer commented Mar 8, 2022

I have updated this PR addressing some of the comments @pp-mo raised, particularly this comment:

Changes:

  • Reran the lockfile generation to pick up eccodes 2.25 - no new test failures due to this
  • Cherry-picked @pp-mo's commit casting units.date2num to float the: 519ca5e

I haven't included changes to using grib_set_missing as I found issues with that as we discussed over the phone

@pp-mo pp-mo merged commit ac658b4 into SciTools:main Mar 8, 2022
@pp-mo
Copy link
Member

pp-mo commented Mar 8, 2022

Thanks @lbdreyer for persisting with this !
This should bring a new release quite close now.

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.

2 participants