Skip to content
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

Re-add 'main' conda-forge channel; fix for eccodes >= 1.19 #240

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Jan 19, 2021

This fixes the RTD docs-build error which emerged following the cut of the original 0.16.0rc0 pre-release tag.

Given the error seen in the attempt to re-spin #224 today, we suspect those errors will come through here too
(something to do with ellipsoid definitions changing)
So we will probably want to wait until that is fixed before merging this ...

@pp-mo pp-mo requested a review from lbdreyer January 19, 2021 12:08
@pp-mo
Copy link
Member Author

pp-mo commented Jan 19, 2021

Ok, so the key problem is that ECMWF have changed a default parameter setting for GRIB1 messages.
I'm not sure quite how this works, but it looks like 'shapeOfTheEarth' used to be defined as '6' but is now '0'.

All the problems are with GRIB1 files in iris-test-data, that now read back differently,
for example <testdata...>/GRIB/rotated_uk/uk_wrongparam.grib1
I can't find any specific reference to the 'shapeOfTheEarth' in the GRIB1 specifications, so I think this is a case where the gribapi returns a grib2-style parameter for a grib1 message "as if" it had that meaning.
Such attempts, to leverage the gribapi "generic" view of data, were our whole problem with the original iris grib interface, which led to its replacement with the 'strict' grib loader (i.e. the code based on GribMessage not GribWrapper).

From comparing release downloads of eccodes, the key change seems to be in a file definitions/grib1/section.2.def
At version 1.18 this reads...

# START grib1::section
# SECTION 2, Grid description section
#  Length of section

position offsetSection2;
 . . .
transient shapeOfTheEarth=6 : hidden;

Whereas in 1.19 this last line changed to ...
transient shapeOfTheEarth=0: hidden; #ECC-811

In the release notes , Bug Fixes section, we have
"[ECC-811] - shapeOfTheEarth=6 for GRIB1"

Unfortunately the issue content may be private -- I can't view it but @lbdreyer can ?

@pp-mo
Copy link
Member Author

pp-mo commented Jan 19, 2021

I've posted a 'fix' to that gribapi change.
From discussion with @lbdreyer, our main priority right now is backwards-compatibility.
We can discuss later whether this was always really wrong and should in future be fixed.

# However, this does not match 'radiusOfTheEarth' and may be incorrect : We may change it in future.
soe_code = 6
if soe_code == 0:
# New supposedly-correct correct value, matches the 'radiusOfTheEarth' parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional, you are hardcoding soe_code to be 6, but including the if soe_code == 0 for future, if we decide to change it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I thought I should remove most of the previous unused options, but I left both these ones in for reference.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 92.827% when pulling 063a6a3 on pp-mo:fix_channels into 5a96b74 on SciTools:v0.16.x.

@lbdreyer
Copy link
Member

Looks good to me!
As @pp-mo says I think we need to prioritise minimal change for this release.
We would first need to understand the impact of changing to the new default (covered by #241), i.e. how many users would it upset as they expect the old, incorrect default, and as we're pressed for time right now, I don't think this is the right time.

@lbdreyer lbdreyer merged commit 9a5859f into SciTools:v0.16.x Jan 20, 2021
@pp-mo pp-mo changed the title Add 'main' conda-forge channel, needed for docs builds. Re-add 'main' conda-forge channel; fix for eccodes >= 1.19 Jan 27, 2021
jamesp added a commit to jamesp/iris-grib that referenced this pull request Feb 23, 2021
* upstream/v0.16.x:
  Docstest 0v16 (SciTools#244)
  Remove eccodes bug workaround added in SciTools#208. (SciTools#224)
  Update requirements to pick up Iris 3. (SciTools#243)
  Add 'main' conda-forge channel, needed for docs builds. (SciTools#240)
  Fix RC date in release notes (about to cut). (SciTools#235)
  Version 0.16 release candidate (SciTools#232)
  Cosmetic change : rename the travis iris-test-version options (SciTools#234)
  Travis test with  both Iris latest-release and latest-master. (SciTools#231)
lbdreyer pushed a commit that referenced this pull request Feb 24, 2021
* Add 'main' conda-forge channel, needed for docs builds.

* Use fixed spherical-earth-radius in GRIB1, ignoring change in gribapi default.

* Codestyle fix.
bjlittle pushed a commit that referenced this pull request Mar 2, 2021
* Travis test with  both Iris latest-release and latest-master. (#231)

* Travis test with  both Iris latest-release and latest-master.

* Modify test CMLs for latest Iris (Iris3.0 changes).

* Grib1 load fixes.

* Fix loading since units=None default for Iris3 coords

* Modify test to work with latest Iris (Iris3.0 changes).

* Test against latest Iris only.

* Review changes.

* Cosmetic change : rename the travis iris-test-version options (#234)

* Rename Iris test-version options, and enable all to check action.

* Review changes.

* Version 0.16 release candidate (#232)

* Require Iris >=3 (just for the gdt90 changes).

* Whatsnew entry for requiring Iris 3.

* Set version string for release candidate.

* Test against Iris 3.0.0rc0 from conda-forge/rc_iris.

* Fix RC date in release notes (about to cut). (#235)

* Add 'main' conda-forge channel, needed for docs builds. (#240)

* Add 'main' conda-forge channel, needed for docs builds.

* Use fixed spherical-earth-radius in GRIB1, ignoring change in gribapi default.

* Codestyle fix.

* Update requirements to pick up Iris 3. (#243)

* Remove eccodes bug workaround added in #208. (#224)

* Docstest 0v16 (#244)

* Document PR#240 in release notes.

* Fix version string and release-notes date.

* Added getting started .cirrus.yml

* Update cirrus to use miniconda image

* Update .cirrus.yml

* Update .cirrus.yml

Copied across cirrus file from iris-ugrid

* Added nox testing

Borrowing from iris and iris-ugrid, added nox testing and a test runner.

Tests currently fail on my local machine.

* Path fixes in cirrus.yml

* eccodes test added to noxfile

* Added config and coverage

* Trying to set SITE_CFG

* syntax error

* taking the IRIS_DIR from Travis CI config

* Add allow_failures to the linux task for now

* Dodgy r key...

* Update .cirrus.yml

* Allow failures in linting

* Force return error 0  for now

* Added eccodes test

* moved matrix

* Yaml syntax error & * wrong way around

* Typo

* Update iris version dependency

* Date fix

This resolves the same bash issue as SciTools/iris#4019

* Configure Iris in nox

* Updated CI config

* correct site-packages path

* Fix to iris test data path

* force cache update

* refactored writing iris config in noxfile

* Fix pep8 and license test fails

* Support for testing against packaged iris and building from source

* Adding yaml to list of cirrus container requirements

* invalidate cirrus cache

* really invalidate cirrus cache...

* yaml -> pyyaml

* missing iris_dir reference

* call write_iris_config

* docstrings and matrix testing

* git syntax error

* eccodes selfcheck in basic tests

* Removed py3.8 testing for now

* Fixed regression in setup.py test

* Removed unused imports

* Disabled lint checking

* Removed nox from test dependencies for python3.6 and python3.8

* Removed python3.8 from noxfile

* Removed nox from test requirements

Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
@pp-mo pp-mo deleted the fix_channels branch March 3, 2022 14:46
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