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

Add recipe for sea ice area and extents in southern polar region #3607

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

flicj191
Copy link
Contributor

@flicj191 flicj191 commented May 22, 2024

Description

New recipe for sea ice area trends and extents around Antarctica. Using code from @anton-seaice for a COSIMA (Consortium for Ocean-Sea Ice Modelling in Australia) 'cookbook' example.


Before you get started

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 recipe/diagnostic

To help with the number of pull requests:

@flicj191 flicj191 changed the title Add recipe for sea ice area and extents in south polar Add recipe for sea ice area and extents in southern polar region May 22, 2024
@flicj191
Copy link
Contributor Author

@rbeucher see draft pr for sea ice recipe

@flicj191 flicj191 added the new recipe Use this label if you are adding a new recipe label Jun 5, 2024
@flicj191 flicj191 marked this pull request as ready for review June 11, 2024 00:29
@flicj191
Copy link
Contributor Author

this recipe could possibly be moved to examples ?

@flicj191
Copy link
Contributor Author

@headmetal @rhaegar325 can you also have a look at this when you get a chance?

@bouweandela
Copy link
Member

@flicj191 Could you please have a look at the checklist and make sure you've addressed all the items as best as you can? Our convention is that the author of the pull request fills in the checklist before asking for a review. If anything is unclear don't hesitate to ask.

@flicj191
Copy link
Contributor Author

Codacy issues were too many local variables and a docstring that matches PEP257 from what I can see. Are these acceptable? @ESMValGroup/esmvaltool-developmentteam

@bouweandela
Copy link
Member

Yes, a few issues should be fine.

flicj191 and others added 4 commits July 15, 2024 14:00
@bouweandela
Copy link
Member

the input data file type is a mystery to me - why use xarray here?

The choice for xarray here seems mostly a personal preference and that is absolutely fine!

However, re-implementing existing preprocessor functions in the diagnostic is something that should preferably be avoided.

@valeriupredoi
Copy link
Contributor

The choice for xarray here seems mostly a personal preference and that is absolutely fine!

In general, I tend to agree with this, but this may lead to a number of issues:

  • we could end up, in the future, with a constant move away from Iris, with other diagnostic developers relying more and more on xarray; so far only four diagnostics use it, so let's not get too impatient, but I'm starting to notice xarray's much higher popularity vs Iris
  • this means using ESMValCore preprocessors will get harder and harder to be used in diags, due to the incompatibility between xarray and Iris IOs

That's why I asked Felicity, if possible, to stick with the oldschool Iris way 🍺

@rbeucher
Copy link
Contributor

rbeucher commented Jul 17, 2024

I agree with @valeriupredoi and @bouweandela that the preprocessors should handle the heavy lifting. Since they're based on IRIS, we're somewhat limited at the moment. Supporting XArray in the future would be great, though I understand it's a complex task.

@bouweandela
Copy link
Member

@rbeucher Please open a GitHub issue if you encounter such limitations. We are applying for funding for future work at the moment, so if we are aware of shortcomings in the preprocessor, we can plan to solve these issues in future projects.

@rbeucher
Copy link
Contributor

Sure, I will. We haven't worked on this recently. Hopefully soon.

@alistairsellar
Copy link
Contributor

Hi @flicj191 if you are still looking for a science reviewer I could ask one of the sea-ice team at the Met Office. I would think that @anton-seaice's review would suffice for science review since he knows what the outputs should look like. But let me know if you think it really needs another science reviewer.

@flicj191
Copy link
Contributor Author

Hi @flicj191 if you are still looking for a science reviewer I could ask one of the sea-ice team at the Met Office. I would think that @anton-seaice's review would suffice for science review since he knows what the outputs should look like. But let me know if you think it really needs another science reviewer.

Thanks @alistairsellar ! I will do a bit more work on this to test some regridders then will get @anton-seaice to have a look at the outputs so sounds good for that to be the science review. Cheers

@alistairsellar
Copy link
Contributor

Thanks @alistairsellar ! I will do a bit more work on this to test some regridders then will get @anton-seaice to have a look at the outputs so sounds good for that to be the science review. Cheers

Sounds good @flicj191 !

@flicj191 flicj191 removed the request for review from a team September 12, 2024 01:57
@flicj191
Copy link
Contributor Author

Hi @bouweandela @valeriupredoi I have restructured the recipe to use the preprocessors ready for re-review

Checking some different regridding preprocessors, This is a plot from previous diagnostic script
map_difference png
with these packages on the main branch I believe:

INFO    [3335387] ESMValCore: 2.11.0.dev8+gb47f7c2ca
INFO    [3335387] ESMValTool: 2.10.0.dev68+gf16c7eab7.d20231122

then with this preprocessor:

  map_extents: &map
    climate_statistics:
      operator: mean
      period: monthly
    extract_region: 
      start_longitude: 0
      end_longitude: 360
      start_latitude: -90
      end_latitude: -50
    regrid:
      target_grid: 0.5x0.5
      scheme: linear

siconc_linear

  map_esmpy:
    <<: *map
    regrid:
      target_grid: 0.5x0.5
      scheme:
        reference: esmf_regrid.schemes:ESMFBilinear

siconc_esmpy
then the target_grid to observations seems to do something strange:

  map_target:
    <<: *map
    regrid:
      target_grid: NSIDC-G02202-sh
      scheme: linear

siconc_target

  map_esmpy_target:
    <<: *map
    regrid:
      target_grid: NSIDC-G02202-sh
      scheme:
        reference: esmf_regrid.schemes:ESMFBilinear
        mdtol: 0.7

siconc_esmpy_target
I'll have a look at the preprocessed files when I can.
@anton-seaice, not sure if there's a specific regridding scheme you would need?

@anton-seaice
Copy link

I expect bilinear interpretation is a good choice. The monthly averages of sea ice concentration should be fairly 'smooth' in space, so biliear is appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
looking for technical reviewer new recipe Use this label if you are adding a new recipe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New recipe for sea ice area trends and extents
6 participants