Updates for CMIP7#1066
Conversation
variables for CMIP7. - Add new variables that were not there before - Change names for some of the variables - Add an f_CICE flag to be able to turn off CICE duplicate variables - Added more variables to the daily stream
anton-seaice
left a comment
There was a problem hiding this comment.
Thanks @dabail10 - I will give this a proper review next week
I dropped a couple of comments - i am not sure about mask_ice_free_points=.true. for extensive variables
In general the cell_methods in the data request are a bit of a mess now, there are several methods more then there used to be and some errors. If we want to be setting them directly in CICE it will take some work (e.g. its more than just one for intensive and one for extensive!)
- add a few more SIMIP variables (sisndmasswind, ...) - remove sialb as this is no longer in the CMIP7 table - start some work on the aice versus aice_init thing
|
Documentation will need to be updated. I'm also confused about the f_CICE and f_CMIP implementation. Why not just set all the CMIP f_variables=f_CMIP? And are all or some of the CMIP variables also exposed independently with f_ namelist and which takes priority and/or should there be an ability to "merge" the independent setting and the f_CMIP value instead of overwriting? If the implementation introduces limitations on the settings, we should make that clear in the documentation. |
This is a good question. This was the way I thought I would deal with some variables only on the monthly stream and some variables on the daily stream with the one flag, f_CMIP. I'm open to other ideas. |
- change avg_ice_present to be a character array - add capability to average when aice_init > 0 - add long_name (title) for SIMIP variables from the CMIP7 table - update some ice density calculations to use the mushy-layer brine+ice density
|
Just added a whole bunch of changes. I think this is getting pretty close. I have a one-month run with the output here. Outstanding issues:
|
This would be great but i can't think of how to implement it within the current framework. (e.g. something like, if the variable is in the ice_in file, then override with the f_CMIP or f_CICE value) My view is that all diagnostics should just be off by default, and then let the user set the ones they want. That would break a lot of configurations though, so takes some care if others agree! |
anton-seaice
left a comment
There was a problem hiding this comment.
Thanks @dabail10
I didn't go through everything, but the main themes are:
- in some places this used fixed values of rhoi, rhos and in others they come from icepack. Is this correct ?
- Melt processes should always be less than 0 in cmip diagnostics
- There's lots of loops that aren't needed once
if aice>punycondition is removed, they can just becall accum_hist_field(n_..., iblk, ...(:,:,iblk), a2D) - Can all the _ai variables be saved before
scale_fluxesis called rather than recalculating them
I have fixed the fllwutop field and the siitd fields. Thoughts on what to put in the comments field? The description from the table? the CF standard_name? |
|
On point 4, this is a larger issue. Ultimately we want to get rid of the _ai quantities. However, that is for a future PR. |
|
Thanks for your work @dabail10 ! Can this calculation be a subroutine or done once in the file and stored, rather than repeated through the code ? : |
Description field can be quite long ? It's a bit of an annoying thing to hard-code . I don't have a strong view |
You know, I bet there is a mushy function for this already ... |
|
I still think there is something inconsistent with snowfrac / sisnconc: Snowfrac is per grid cell area CICE/cicecore/cicedyn/analysis/ice_history.F90 Lines 686 to 689 in d0455f6 Sisnconc is per sea ice area (and extensive) CICE/cicecore/cicedyn/analysis/ice_history.F90 Lines 1780 to 1784 in d0455f6 But they are calculated the same: CICE/cicecore/cicedyn/analysis/ice_history.F90 Lines 2401 to 2402 in d0455f6 CICE/cicecore/cicedyn/analysis/ice_history.F90 Lines 2818 to 2820 in d0455f6 |
Ah, I think I understand. The variable snowfrac is only the fraction on the sea ice. This comment is incorrect. |
|
In icepack we have the following: Where fsn is the snow fraction on the sea ice per category and this is aggregated like apeff_ai. I will modify the comment. |
|
Getting a lot of compare fails. |
It looks like your branch was created off a version of CICE on Nov 12. There was a commit to main on Nov 26 that changed answers. You either need to compare to the version of CICE you branched from or update your branch to something more recent. If this is ready, I can run some tests again. What I do is checkout CICE main then merge the PR branch into my sandbox. This duplicates what will happen when we merge the PR and also ensures testing is done relative the latest main version. You could create a sandbox like this was for testing, you do not necessarily have to update your branch to the latest CICE main. Just to check again, do we think modifications are complete at this point. If so, I will test too. |
|
Oh shoot. I forgot about that. I will update the base branch. |
|
There it was a clean update with no conflict. I will rerun my tests. |
|
I am now getting some compile errors. I guess the merge was not as clean as I hoped. /glade/work/dbailey/CICE_ESCOMP/cicecore/cicedyn/general/ice_step_mod.F90(685): error #6627: This is an actual argument keyword name, and not a dummy argument name. [WAVE_HEIGHT_TYPE_OUT] I didn't update icepack appropriately. |
|
Much better. 3 PEND and 2 FAIL. I think this is expected for the hdf5/cdf tests? |
Yes, that looks like what we expect. |
|
Are you both done with your testing? @dabail10 have you gone through all of the history fields to make sure they look reasonable? Before it's merged, please add PR header statements like "closes issue #[whatever]" so that we don't have to close them manually. It would be nice to merge this today for weekend testing! |
I have looked through all the fields numerous times and I am happy. I have copied one year of data to here: https://ftp.cgd.ucar.edu/pub/dbailey/cicecmip/ It would be great if others had a look in case I missed something. My io_suite test did as expected. I will investigate how to do the automatic closing of issues. |
|
I have run a partial test suite on Derecho with 4 compilers and feel like this is ready to be merged. I will merge this afternoon unless someone does it before me or anyone notes any last minute concerns. |
|
So, are there five issues that should close? "To automatically close issues when merging a pull request (PR), use specific keywords like It looks like you added the issues that are related, please update the PR description to explicitly add phrasing like closes #123 for any issue that should automatically close. |
Added. I thought if they were linked it would do the same. |
Maybe they do, it's hard to know for sure. Thanks for adding that text. |
|
Merged. Yay! Take the weekend off, will you? :) |
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Starting this in draft mode. This is a big PR with all of the SIMIP updates for CMIP7.
dabail10 (D. Bailey)
io_suite results here
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#6b7cd825797762e22fe0a05342917d574a980966
This PR should address issue #1048 and #1038.
Closes #904
Closes #1033
Closes #1038
Closes #1048
Closes #1057