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

CMORizer for JRA-55 #3141

Merged
merged 22 commits into from
Oct 1, 2024
Merged

CMORizer for JRA-55 #3141

merged 22 commits into from
Oct 1, 2024

Conversation

axel-lauer
Copy link
Contributor

@axel-lauer axel-lauer commented Mar 28, 2023

Description

This PR adds downloading and formatting scripts for the JRA-55 reanalysis. JRA-55 data have been provided long ago as ana4mips dataset that is no longer available (or maintained). In order to add new variables, the original JRA-55 data have to be CMORized. The original data are only available as GRIB1.

Note: This PR requires the Python library cfgrib to process the GRIB files, e.g. conda install cfgrib

This PR allows processing of the following JRA-55 variables:

  • cli
  • clivi
  • clw
  • clwvi
  • clt
  • prw
  • rlus
  • rlut
  • rlutcs
  • rsus
  • rsuscs
  • rsut
  • rsutcs
  • tas
  • wap

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated data reformatting script

@alistairsellar
Copy link
Contributor

Hi @axel-lauer are you still looking for a science reviewer for this one? I could do this...

@hb326
Copy link
Contributor

hb326 commented Jul 20, 2023

Hi @axel-lauer are you still looking for a science reviewer for this one? I could do this...

@alistairsellar: Axel is on vacation right now, but I know that he is still looking for a science reviewer for this. So thanks for offering! :)

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Nice work, @axel-lauer! The code looks good to me and all works fine: downloader, formatter, recipe_check_obs. I only have one suggestion for a codacy issue, the others could be ignored. The downloading script follows recommendations on the source server and worked fine for me.

I would have given a technical approval if the tests were still passing but that is no longer the case after the main branch was last merged into this one. There seems to be an issue with the iris-grib package. It could be an issue with an upstream package rather than how the package is used in this CMORizer. I will try to take a deeper look next week and report here what I could figure out.

esmvaltool/cmorizers/data/formatters/datasets/jra_55.py Outdated Show resolved Hide resolved
@remi-kazeroni
Copy link
Contributor

I could get the dependency tests pass locally by adding python-eccodes<1.6.0 to environment*yml and setup.py which is needed to install iris-grid. The tests would still fail locally with python-eccodes not pinned (i.e. 1.6.0).

For the latest version of python-eccodes, I still get ImportError: /opt/conda/envs/esmvaltool/lib/python3.11/site-packages/gribapi/_bindings.cpython-311-x86_64-linux-gnu.so: undefined symbol: codes_bufr_key_is_coordinate which seems to be an issue upstream.

If I understand correctly, the python-eccodes would need to be rebuilt on conda-forge to be compatible with the latest eccodes (2.30.2, but still at 2.21.2 on conda-forge). But perhaps our in-house conda-forge specialists, @zklaus and @valeriupredoi, would know better how to best handle eccodes and the Python bindings for the Tool 🍻

Copy link
Contributor

@alistairsellar alistairsellar left a comment

Choose a reason for hiding this comment

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

Hi @axel-lauer looks good for science review. Or at least I'm happy with code (pending response to suggestions) & doc changes (can you link documentation to PR description).

Should science review cover some looking at output files? Presumably I would need to make a new environment to run this, as standard esmvaltool modules (e.g. on Jasmin) won't yet have iris-grib?

esmvaltool/cmorizers/data/formatters/datasets/jra_55.py Outdated Show resolved Hide resolved
esmvaltool/cmorizers/data/formatters/datasets/jra_55.py Outdated Show resolved Hide resolved
utils.fix_var_metadata(cube, cmor_info)

# fix z-coordinate (if present)

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant blank line?

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the other changes @axel-lauer. Not sure if this one got missed or you want to keep the blank line for a reason? It would now be line 135 that this refers to.

@zklaus
Copy link

zklaus commented Sep 1, 2023

I could get the dependency tests pass locally by adding python-eccodes<1.6.0 to environment*yml and setup.py which is needed to install iris-grid. The tests would still fail locally with python-eccodes not pinned (i.e. 1.6.0).

For the latest version of python-eccodes, I still get ImportError: /opt/conda/envs/esmvaltool/lib/python3.11/site-packages/gribapi/_bindings.cpython-311-x86_64-linux-gnu.so: undefined symbol: codes_bufr_key_is_coordinate which seems to be an issue upstream.

If I understand correctly, the python-eccodes would need to be rebuilt on conda-forge to be compatible with the latest eccodes (2.30.2, but still at 2.21.2 on conda-forge). But perhaps our in-house conda-forge specialists, @zklaus and @valeriupredoi, would know better how to best handle eccodes and the Python bindings for the Tool 🍻

Thanks for bringing this up! I dug a bit deeper and it looks like this python-eccodes upstream (at ECMWF) declares a min_recommended_version for Eccodes which currently stands at 2.21.0. However, with release 1.6.0 they added linking to a function that was only added to Eccodes in 2.31.0. Conda-forge relies on the declaration in python-eccodes to do the pinning, so I have opened an issue there (ecmwf/eccodes-python#82) to update that. In the meantime, we can already fix the conda-forge package, which is tracked in conda-forge/python-eccodes-feedstock#86.

Could you possibly test if things work with python-eccodes-1.6.0 and eccodes-2.31.0?

@axel-lauer axel-lauer requested a review from a team as a code owner September 27, 2023 11:41
Copy link
Contributor

@alistairsellar alistairsellar left a comment

Choose a reason for hiding this comment

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

Thanks for making changes @axel-lauer. Apart from one outstanding question about a blank line (see above), I think that covers my review, so marking as approved. Great stuff!

@axel-lauer
Copy link
Contributor Author

Thanks for your review @alistairsellar ! I removed the blank line as suggested.

@schlunma
Copy link
Contributor

Could you possibly test if things work with python-eccodes-1.6.0 and eccodes-2.31.0?

@zklaus let's discuss this in ESMValGroup/ESMValCore#2178, it seems this PR is no longer using iris-grib.

@axel-lauer
Copy link
Contributor Author

Could you possibly test if things work with python-eccodes-1.6.0 and eccodes-2.31.0?

@zklaus let's discuss this in ESMValGroup/ESMValCore#2178, it seems this PR is no longer using iris-grib.

This is correct, I replaced iris-grib with xarray/cfgrib, which is able to distinguish between different level types (in contrast to iris-grib). This makes the code much easier to read and maintain.

Copy link
Contributor

@flicj191 flicj191 left a comment

Choose a reason for hiding this comment

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

Data downloaded and CMORized on Gadi, thanks


os.makedirs(downloader.local_folder, exist_ok=True)

user = os.environ.get("rda-user")
Copy link
Contributor

Choose a reason for hiding this comment

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

Authentication methods may have changed on RDA, my login uses my orcid login, so this user, password wasn't working for me. But I commented out all the downloader.login() (lines 43-56, 64-69) stuff and was able to download fine on Gadi supercomputer.

Copy link
Contributor

@rbeucher rbeucher left a comment

Choose a reason for hiding this comment

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

Had a look at this and it looks good to go

@rbeucher rbeucher merged commit 6155acf into main Oct 1, 2024
8 of 9 checks passed
@rbeucher rbeucher deleted the cmorizer_jra-55 branch October 1, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants