Skip to content

CMORizer ESACCI-PERMAFROST#3545

Open
axel-lauer wants to merge 32 commits into
mainfrom
cmorize_esacci_permafrost
Open

CMORizer ESACCI-PERMAFROST#3545
axel-lauer wants to merge 32 commits into
mainfrom
cmorize_esacci_permafrost

Conversation

@axel-lauer

Copy link
Copy Markdown
Contributor

Description

This PR adds a downloading and formatting script for the dataset ESACCI-PERMAFROST v3.0. The following three variables are supported:

  • active layer thickness (alt)
  • permafrost ground temperature (gtd)
  • permafrost extent (pfr)

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

@LisaBock LisaBock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread esmvaltool/cmorizers/data/downloaders/datasets/esacci_permafrost.py Outdated
Comment thread esmvaltool/cmorizers/data/cmor_config/ESACCI-PERMAFROST.yml Outdated
@axel-lauer axel-lauer added this to the v2.14.0 milestone Jan 12, 2026
@jlenh jlenh added in technical review requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. labels Feb 6, 2026
@jlenh jlenh modified the milestones: v2.14.0, v2.15.0 Feb 13, 2026

@jlenh jlenh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread esmvaltool/cmorizers/data/cmor_config/ESACCI-PERMAFROST.yml Outdated
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py Outdated
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py Outdated
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py Outdated
@axel-lauer

Copy link
Copy Markdown
Contributor Author

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.

I just updated with the latest main: 6f09965

For remaining Codacy issues, I would appreciate some help.

@valeriupredoi

Copy link
Copy Markdown
Contributor

@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 🍺

@axel-lauer

Copy link
Copy Markdown
Contributor Author

@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 os with ones to Path. There are two remaining two Codacy issues: "too many branches" and "too many statements". I will ignore these. This is a simple CMORizer, no software for space rockets.

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 jlenh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 :)

Comment thread esmvaltool/cmorizers/data/downloaders/ftp.py
Comment thread esmvaltool/cmorizers/data/formatters/datasets/esacci_permafrost.py
@axel-lauer

Copy link
Copy Markdown
Contributor Author

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.

@jlenh

jlenh commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

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 :)

@rbeucher

Copy link
Copy Markdown
Contributor

Hi all — we’re getting ready for the next ESMValTool release, version 2.15, and are planning to freeze the code on 26 June 2026.

Could you please let us know whether you think you’ll be able to finalise the work in this PR by then? If not, no problem — it would just help us plan what can realistically make it into the release.

@flicj191 tagging Felicity for visibility.

@flicj191

Copy link
Copy Markdown
Contributor

Hi all, as the associated PR ESMValGroup/ESMValCore#2358 containing the required custom CMOR tables is not merged yet and has some outstanding comments, this may need to be moved to the next milestone.
Please let me know if this will cause any issues? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved by scientific reviewer approved by technical reviewer observations requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request..

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants