-
Couldn't load subscription status.
- Fork 149
Add an option to be able to update ice concentration (from data assimilation or retrievals/products) #1060
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
base: main
Are you sure you want to change the base?
Conversation
…restart_fh") via UFS configuration as for other components Part of ufs-community/ufs-weather-model#2348
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assimilation scheme moves modeled ice towards observed ice when some requirements are fulfilled. It is implemented in the restart driver and acts as a correction to the restart file.
It has similarities to what is done at DMI. The requirements are to some extend hard coded and I think that the inclusion would benefit from namelist control of for instance when observations and modeled values are close enough in order not to be changed. This may result in some namelist parameters.
I have also added some specific comments in the text.
Thanks @TillRasmussen for your comments and suggestions. I'll improve to the extent possible with a few commits.
Is the code/option as used at the DMI already available to use? If so, I will close this PR and use that instead. FYA: @NickSzapiro-NOAA if @TillRasmussen confirms this is the case, can you please help? |
…ad/close}_nc. While there, for sake of logging, also echoing the name of the file that is being read in.
|
My point with the similarities is that both your system and the DMI system has a lot of if conditions that determines whether or not to assimilate. These are based on experience and test with the system and the type of observations. It would be nice to be able to have these as a common set of namelist parameters, however I think that others should have a say in that as it will easily add ~5 namelist parameters (or more). An example could be: The DMI code is not by default public available but we can probably share it. We jut have to entangle a few DMI modifications away from it that is not related to this.
My suggestion would be that you have something that works or almost works and get it into the system. I would like opinions from others on how to deal with the namelist parameters etc. |
corrections to insert_sic
@TillRasmussen, thanks for reviewing and suggestions.
|
|
Hi @sanAkel
|
@lbertino Glad to hear from you and thanks for chiming in.
This is great progress at your end! 🏆 👏 |
|
Checking if anything is pending on this PR: @apcraig ? |
|
The implementation looks pretty clean to me at this point. I appreciate the updates that were done and resuse of existing infrastructure. Thanks for all the earlier comments from others and all the work. The new subroutine, direct_insert_sic, does have a lot of hardwired logic. I'm not the right person to suggest what "science" parameters should become namelist. If there is consensus about what to do, that's great, maybe do it now. It could also come in as a separate PR. My only comment might be that insert_sic as a logical namelist might be limiting. Do we want to have a string instead of a logical, basically supporting multiple options via a single namelist. So false would be "none" or "off" and true would be "adjust_aice" or similar. "sic" isn't really a CICE term, but I'm OK using it if others like it, but maybe aice is the better term for CICE? And maybe the namelist could be "modify_restart" instead of "insert_sic" as well? So modify_restart = "adjust_aice" instead of insert_sic = .true. providing extensibility in the future? With the idea that we might have "adjust_aice_hi" or "adjust_aicev2" or other options later. It's better to implement flexible namelist now to avoid changing namelist later, especially if that namelist needs to be removed. The other thing is that maybe we want a namelist to specify the input file instead of hardwiring it in the code to 'sic.nc'. Maybe that namelist could be called "modify_restart_filename"? This is less critical because we could also add it later without any backwards compatibility issues. |
|
Thanks @apcraig for reviewing and suggestions.
👍
If other reviewers ( @TillRasmussen, @eclare108213 ?) also suggest same/similar, to move this PR along, (1) I can change the logical (still boolean flag, unless someone asks for otherwise):
(2) Instead of hard coded read of Please let me know if should proceed with (1) and (2)? |
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
This feature is being ported from CICE4, that is used in RTOFS:
ON(default:OFF) via a logical:insert_sicvariable.insert_ssmi.A brief text/description follows:
One of the main goals of a data assimilation (DA)/prediction system that strives to provide close to observed sea ice concentrations and ice edge is to be able to reconcile the differences in modeled and observed sea ice concentrations derived from space borne instruments. Posey et al., 2025 implemented such a method with CICE4, it is hereby ported to CICE6. By default this feature is turned
OFF. To turn itON, setinsert_sicto.true.inice_in(input) namelist file and providesic.ncto be able to nudge the modeled ice concentration to that from data assimilated (or observed) concentration.Developer(s):
@awallcraft
Suggest PR reviewers from list in the column to the right.
I'm unable to do that. Alerting @apcraig for his help.
Please copy the PR test results link or provide a summary of testing completed below.
Tested against the
developbranch of the NOAA-EMC CICE fork: https://github.com/NOAA-EMC/CICE.gitTest was
datm_cdeps_mx025_gefs. At orion/hercules, paths:developbranch):/work/noaa/stmp/santa/stmp/santa/FV3_RT/rt_3352132/datm_cdeps_mx025_gefs_intel/BASL/sanAkel:alan_ssmibranch, i.e., this PR) withinsert_sic= .false.:/work/noaa/stmp/santa/stmp/santa/FV3_RT/rt_3352132/datm_cdeps_mx025_gefs_intel/CTRLinsert_sic= .true.):/work/noaa/stmp/santa/stmp/santa/FV3_RT/rt_3352132/datm_cdeps_mx025_gefs_intel/EXPResults:
RESTART/iced.2011-10-02-00000.nc.How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on Icepack or any other models?
Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
Does this PR add any new test cases?
Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.
Further details on results:
1-day.insert_sicRemarks:
insert_ssmi. @awallcraft ported the code he wrote (and is used in CICE4) to CICE6.