Conversation
LisaBock
left a comment
There was a problem hiding this comment.
Thanks @axel-lauer !
I started the review and run in two issues:
- there were small modifications in the downloader necessary (see comment there)
- when starting the formatter I got the following error message:
PermissionError: [Errno 13] Permission denied: '/work/bd0854/b380103/esacci-permafrost-weights'
Please update also the branch with the main branch.
esmvaltool/cmorizers/data/downloaders/datasets/esacci_permafrost.py
Outdated
Show resolved
Hide resolved
|
Hello, this pull request has been marked with the If you won't be able to finish this in time, don't worry - just unassign the milestone |
There was a problem hiding this comment.
Looks good overall, just a few comments!
If you could also deal with the Codacy issues! It looks like mostly using the Path library instead of os, and some looping over dictionary keys. Let me know if you need assistance with that 😄 And there is a conflict to solve with the main branch.
The tests are failing because of the dependency on the newest ESMValCore version. I have also bumped the PR to the next version as it seems that the needed PR in ESMValCore will not be merged for v2.14.0.
esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
Outdated
Show resolved
Hide resolved
esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
Outdated
Show resolved
Hide resolved
esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
Outdated
Show resolved
Hide resolved
I just updated with the latest main: 6f09965 For remaining Codacy issues, I would appreciate some help. |
|
@axel-lauer please fix the two failing tests. also for Codacy stuff, datetime objects should ideally contain timezone info, see https://docs.astral.sh/ruff/rules/call-datetime-without-tzinfo/ - in fairness, that#s probably not needed for us, but I don't think it hurts adding an eg UTC timezone either 🍺 |
Thanks @valeriupredoi for taking a look! I added the time zone info and replaced calls to The tests of recipe_check_obs.yml are failing because PR ESMValGroup/ESMValCore#2358 containing the required custom CMOR tables is not merged yet. |
jlenh
left a comment
There was a problem hiding this comment.
Looks good @axel-lauer, thanks for the modifications! I added two small comments.
Otherwise I tested the cmorizer on a few years of data and it produces correct outputs.
The remaining failing test is indeed due to the core PR that needs to be merged first, I tested it in dev mode and the recipe_check_obs works fine then (apart from the missing years in the files I have cmorised on my side) Maybe you can add the cmorised dataset in the data pool then to fully test :)
Thanks @jlenh ! I just moved the CMORized files to our obs-pool on Levante. |
Great! Then we will come back to this once the Core PR is merged :) |
Description
This PR adds a downloading and formatting script for the dataset ESACCI-PERMAFROST v3.0. The following three variables are supported:
The CMORizer requires the custom CMOR tables defined in ESMValGroup/ESMValCore#2358.
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