ecmwf / cfgrib

A Python interface to map GRIB files to the NetCDF Common Data Model following the CF Convention using ecCodes
Apache License 2.0
407 stars 77 forks source link

Refactor Makefile to sync with GitHub Actions #261

Closed alexamici closed 3 years ago

alexamici commented 3 years ago

Integration testing is done in GitHub Actions since quite some time and both the Dockerfile and the Makefiles are left with a lot of confusing unused commands.

In this PR I removed the whole docker support and cleaned up the Makefile to be in sync with the command executed in the GA.

codecov-commenter commented 3 years ago

Codecov Report

Merging #261 (0eea204) into master (abed74b) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #261   +/-   ##
=======================================
  Coverage   96.41%   96.41%           
=======================================
  Files          26       26           
  Lines        1814     1814           
  Branches      211      211           
=======================================
  Hits         1749     1749           
  Misses         39       39           
  Partials       26       26           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update abed74b...0eea204. Read the comment docs.

iainrussell commented 3 years ago

@alexamici , this looks much simpler :) Was this a typo under doc-test though?

python -m pytest -v README.rst

For me, this causes 'make test' to barf.

alexamici commented 3 years ago

@iainrussell make doc-test runs smoothly on my system, what error do you get?

iainrussell commented 3 years ago

@alexamici , OK, I realised that my first error was related to not having ecCodes in my path, but after that was sorted, I get differences in output:

README.rst::README.rst FAILED                                                                                                                                                                        [100%]

================================================================================================= FAILURES =================================================================================================
___________________________________________________________________________________________ [doctest] README.rst ___________________________________________________________________________________________
087
088     $ pip install xarray
089
090 In a Python interpreter try:
091
092 .. code-block: python
093
094 >>> import xarray as xr
095 >>> ds = xr.open_dataset('era5-levels-members.grib', engine='cfgrib')
096 >>> ds
Differences (unified diff with -expected +actual):
    @@ -1,4 +1,4 @@
     <xarray.Dataset>
    -Dimensions:        (number: 10, time: 4, isobaricInhPa: 2, latitude: 61, longitude: 120)
    +Dimensions:        (isobaricInhPa: 2, latitude: 61, longitude: 120, number: 10, time: 4)
     Coordinates:
       * number         (number) int64 0 1 2 3 4 5 6 7 8 9
    @@ -19,3 +19,3 @@
         Conventions:             CF-1.7
         institution:             European Centre for Medium-Range Weather Forecasts
    -    history:                 ...
    +    history:                 2021-10-18T16:11 GRIB to CDM+CF via cfgrib-0.9.9...

/Users/iain/dev/git/cfgrib/README.rst:96: DocTestFailure
========================================================================================= short test summary info ==========================================================================================

The difference in the order of dimensions could be related to which version of ecCodes we use (maybe?), but the fact that the 'history' has the current time stamp means that it cannot generate the exact same output as stored in README.rst. Maybe there's something wrong with my setup if you don't see the same problems.

alexamici commented 3 years ago

but the fact that the 'history' has the current time stamp means that it cannot generate the exact same output as stored in README.rst

@iainrussell the change in the history tag is ignored because the README matches the ellipsis sign .... Annoyingly you still see the difference reported when there is a real difference nearby.

In this case the error is the different order of the dimensions and I'm a bit surprised. There was a change some time ago, are you sure you are using the master cfgrib? Maybe you could just re-run pip install -e . to be sure. Unfortunately the version in the history attribute was cut one character too soon :)

In any event this is an unrelated issue and the PR works as expected in the GA, so I'm going to merge it shortly unless you object.

iainrussell commented 3 years ago

Yea, ok I agree, no problem! I'll double check my version later today.