COSIMA / om3-utils

Mozilla Public License 2.0
0 stars 1 forks source link

Cice grid #5

Closed anton-seaice closed 2 months ago

anton-seaice commented 3 months ago

Following on from https://github.com/COSIMA/om3-scripts/pull/8 but now with tests!

Created a cice grid generation script, using the esmgrids package.

The script creates a cice grid from the mom supergrid and the ocean mask file. The scripts adds the git commit of this script and the input path and md5sum of the mom files to the netcdf output. I added as close as we can get to cf-compliance and a CRS (See https://github.com/COSIMA/om3-scripts/issues/7).

The differences between versions are in ... notebook and explored below in more detail. By the look of things, 1 deg and 0.25 degree files are inherited / historical and 0.1 deg was made for OM2 using the esmgrids package.

  1. Mismatched 0.25 degree ulat/ulon and also in 1 degree ulat/ulon corner . These should be fixed now.
  2. uarea - The 1deg and 0.25deg (i.e. inherited) uareas had some discepancy in them at the join. The new ones appear to be correct, see https://github.com/COSIMA/om3-scripts/pull/6#issuecomment-1980124729. The rest of the uarea in 0.25 deg doesn't match well however, and it looks like it was just set equal to tarea!
  3. AngleT. Only 0.1 degree was wrong and it was being set the same as angle U. This is being updated in esmgrids to use the correct angle from the MOM supergrid. (For completeness, although I think its unrelated, there was an upstream change between CICE5 and CICE6 to use a vector average to calculate angleT rather than a numerical average. The only details given seem to say it was a bug fix so that angles are averaged correctly. The method used in this PR is to get angleT from the MOM supergrid.)

Differences ( I filtered differences less than 2e-6):

1deg new vars not in old? {'crs'} missing vars in new? {'latt_bonds', 'lonu_bonds', 'hue', 'hun', 'latu_bonds', 'lont_bonds'} tlon anom min: nan, anom max: nan angleT anom min: nan, anom max: nan tlat anom min: nan, anom max: nan uarea anom min: -1186349698.6215067, anom max: 33617654.79803729 hte anom min: nan, anom max: nan ulon anom min: nan, anom max: nan tarea anom min: -19085417.985637665, anom max: 9877771.11514306 angle anom min: nan, anom max: nan htn anom min: nan, anom max: nan ulat anom min: nan, anom max: nan 025deg new vars not in old? {'crs'} missing vars in new? set() tlon anom min: nan, anom max: nan angleT anom min: nan, anom max: nan tlat anom min: nan, anom max: nan uarea anom min: -69552757.44506374, anom max: 7366673.800787419 hte anom min: nan, anom max: nan ulon anom min: 3.620558794636963e-05, anom max: 3.141592653589793 tarea anom min: nan, anom max: nan angle anom min: -1.5707963267948966, anom max: 1.5707963267948966 htn anom min: nan, anom max: nan ulat anom min: -0.0018422347075643941, anom max: 0.0018422347075643941 01deg new vars not in old? {'crs'} missing vars in new? {'clat_u', 'clon_t', 'clat_t', 'clon_u'} tlon anom min: nan, anom max: nan angleT anom min: -3.139931603248902, anom max: 1.1133153100325133 tlat anom min: nan, anom max: nan uarea anom min: nan, anom max: nan hte anom min: nan, anom max: nan ulon anom min: nan, anom max: nan tarea anom min: nan, anom max: nan angle anom min: nan, anom max: nan htn anom min: nan, anom max: nan ulat anom min: nan, anom max: nan

image

Even ignoring the join, its not clear why 0.25 deg uarea has changed, but it looks like it was set equal to tarea in the old grid

Screen Shot 2024-03-20 at 4 43 47 pm Screen Shot 2024-03-20 at 4 51 32 pm

Known issues:

  1. Patterning in htn/hte in mom grid? - https://github.com/COSIMA/access-om2/issues/271

See companion PR for esmgrids repo

To-do:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 96.98%. Comparing base (366ac95) to head (29c2291). Report is 2 commits behind head on main.

:exclamation: Current head 29c2291 differs from pull request most recent head 76665f7. Consider uploading reports for the commit 76665f7 to get more accurate results

Files Patch % Lines
om3utils/cice_grid.py 81.08% 7 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5 +/- ## =========================================== - Coverage 100.00% 96.98% -3.02% =========================================== Files 3 5 +2 Lines 190 232 +42 =========================================== + Hits 190 225 +35 - Misses 0 7 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

anton-seaice commented 3 months ago

@aekiss - Sorry - third time lucky for this PR! Can you review please? @micaeljtoliveira @ezhilsabareesh8 - Your reviews are welcome if you'd like

anton-seaice commented 3 months ago

I did only very short comparisons using a 1 degree config in OM2. The sea ice area and volume look practically the same:

image image

Although not identical (anomaly in aice of new grid - old grid after 1 year): image

I ran the 0.25 om2 for 10 years with the new grid. I didn't do a comparison run, but did compare to some save runs. Again nothing particularly of note in sea ice area and volume.

image image

There are more differences in concentration, although I think these will be due to the change in land mask between runs:

image

Link for reference: https://github.com/anton-seaice/sandbox/blob/c745853d63766ca0bb0d7ac477c52d78c3ad69cf/new_cice_grid/new-grid-intake-cat.ipynb

aekiss commented 3 months ago

Thanks @anton-seaice this looks awesome. Thanks especially for the detailed documentation and provenance.

For clarity, should this bit in your initial post

  1. Mismatched 0.25 degree ulat/ulon corner and also in 0.1 degree ulat/ulon. These should be fixed now.

be changed to

  1. Mismatched 1 degree ulat/ulon corner and also in 0.25 degree ulat/ulon. These should be fixed now.

to match the link targets?

micaeljtoliveira commented 3 months ago

I propose moving all of this functionality into the esmgrids package. It certainly makes sense for the tests to live there, and I don't see why all the metadata couldn't be added there.

That makes sense. esmgrids seems to be in need of an update regarding installation and testing. I can have a look into that if you want.

dougiesquire commented 3 months ago

That makes sense. esmgrids seems to be in need of an update regarding installation and testing. I can have a look into that if you want.

Yeah, I think there's a little bit of work needed across the board (both infrastructure and code). That would be great if you can look at installation and testing. If we're going to use esmgrids in other projects, we'll probably want it to be available via conda.

anton-seaice commented 3 months ago

Thanks @dougiesquire

I mostly agree, I want to be sure that we aren't breaking anything historical (e.g. for OM2) and note that we would need to tidyup the old testing setup etc in esmgrids. There also is some overlap / redundancy with mesh generation (om3-scripts).

Hopefully we can be a little bit careful with metadata, especially the CRS, to make sure we can support regional and non-mercator grids in the future.

Can we move this to a new issue, so we can use this PR to make a cice grid for OM2 release and to get us set-up to do testing with esmgrids ?

dougiesquire commented 3 months ago

There also is some overlap / redundancy with mesh generation (om3-scripts).

Agreed. I thought I opened an issue about this, but I can't find it now... Anyway, I've just opened this: https://github.com/COSIMA/om3-scripts/issues/11

Can we move this to a new issue, so we can use this PR to make a cice grid for OM2 release and to get us set-up to do testing with esmgrids ?

So you would like to add this to om3-utils with the intention of removing shortly? Are you okay with this @micaeljtoliveira. The other option is to push hard to get testing running properly on esmgrids. That should be pretty straightforward. Or am I missing something?

micaeljtoliveira commented 3 months ago

Are you okay with this @micaeljtoliveira.

Not ideal, but also not a big deal.

The other option is to push hard to get testing running properly on esmgrids. That should be pretty straightforward. Or am I missing something?

Sorry, I got distracted with the profiling stuff. I'll get to it ASAP. Should indeed be straightforward.

anton-seaice commented 3 months ago

FYI: There are some historical tests in the esmgrids repo, but they rely on some test data which I don't think we have anymore.

micaeljtoliveira commented 3 months ago

FYI: There are some historical tests in the esmgrids repo, but they rely on some test data which I don't think we have anymore.

Assuming the tests were passing last time there were any changes in the repo, one could simply take some random grid and regenerate the test data, no?

anton-seaice commented 3 months ago

FYI: There are some historical tests in the esmgrids repo, but they rely on some test data which I don't think we have anymore.

Assuming the tests were passing last time there were any changes in the repo, one could simply take some random grid and regenerate the test data, no?

Looks like it for the MOM to cice grid:

https://github.com/COSIMA/esmgrids/blob/514b560adf7b9094081408da49ecb3e19ef0353b/test/test_grids.py#L100

I've replaced this with using the _ocean_gridgenerator in this PR

I don't know what this input is though:

https://github.com/COSIMA/esmgrids/blob/514b560adf7b9094081408da49ecb3e19ef0353b/test/test_grids.py#L115

(p.s. we can definitely just leave it failing for now.)

dougiesquire commented 3 months ago

Sorry, I got distracted with the profiling stuff. I'll get to it ASAP. Should indeed be straightforward.

Sorry, I wasn't meaning to suggest things should be happening faster. I was trying (unclearly) to ask whether getting testing running on esmgrids is the main hurdle to moving the code in this PR into esmgrids?

anton-seaice commented 3 months ago

Mostly i'm worried it might send us down a rabbit hole for a while :)

dougiesquire commented 3 months ago

FYI @anton-seaice, Aidan joined our team meeting yesterday. Their plan is to release the 1deg and 0.1deg configs this week and then follow up with the 025deg configs next week once this is all bedded down. So there's a little bit of time.

anton-seaice commented 3 months ago

@aekiss ... I responded to the comments. Looks like well move this to esmgrids but let me know if the new changes make sense

anton-seaice commented 2 months ago

This is being implemented via https://github.com/COSIMA/esmgrids/pull/6