Enable prescribed ice for UFS #964
Conversation
* Remove #ifdef CESMCOUPLED from cicecore/drivers/nuopc/cmeps/CICE_RunMod.F90 * Remove #ifdef CESMCOUPLED from cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 * Testing changing stream_taxmode from cycle to extend * Change include mpif.h to use mpi in ice_prescribed_mod.F90 * Call dshr_pio_init from ice_prescribed_init * Call ice_prescribed_run after ice_prescribed_init * Incorporate ice_prescribed_nml in ice_prescribed_mod.F90 * write stream_taxmode to log. typo in ice_prescribed_mod.F90. * Remove CESMCOUPLED so models initialize ice_prescribed at end of ice_prescribed_init * Change indent for #ifndef for gnu debug * Only call ice_prescribed_run in ice_prescribed_init #ifndef CESMCOUPLED
|
The dependency on CDEPS is not a problem in UFS, but I'm not sure the best way to handle it here. Maybe a different #ifdef "NO_CDEPS" is needed in cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 ? |
| use ice_kinds_mod | ||
| use shr_nl_mod , only : shr_nl_find_group_name | ||
| use dshr_strdata_mod , only : shr_strdata_type, shr_strdata_print | ||
| use dshr_strdata_mod , only : shr_strdata_init_from_inline, shr_strdata_advance | ||
| use dshr_methods_mod , only : dshr_fldbun_getfldptr | ||
| use dshr_mod , only : dshr_pio_init |
There was a problem hiding this comment.
These come from the CIME shr stuff I believe. This is how we do the reading from stream files.
|
Thanks Dave! So most of the code in cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 was enclosed in an #ifndef CESMCOUPLED else: Currently, functions now "uncovered" would need these files for CDEPS inline functionality: One option is renaming the CESMCOUPLED here to something more general |
|
If I am reading this correctly, there were already some depedencies on CDEPS (i.e. other |
|
There was only a dependency on CDEPS when CESMCOUPLED was defined. I guess my concern is that we have code duplication here. Will we need two copies of the CDEPS library? What happens when they get out of sync? |
| #ifndef CESMCOUPLED | ||
| ! If need initial cice values for coupling | ||
| call ice_prescribed_run(idate, msec) | ||
| #endif |
There was a problem hiding this comment.
UFS needed this call ice_prescribed_run at the end of ice_prescribed_init. I don't know if it's a bug for CESM not to. Presumably, the CICE state should be initialized in init, but it may depend on the components' run sequence. We left the #ifndef CESMCOUPLED so as not to impact CESM @dabail10
|
The ice prescribed code is only useful if CICE is linked with CDEPS. I didn't think including CDEPS with CICE was an option. I'm happy to block it all off if not...maybe #ifdef CICE_HAS_CDEPS (?) |
|
I guess I am confused. I understand that you want to use the prescribed ice and hence CDEPS. What I think you are saying is that you would have a separate set of modules / subroutines from CDEPS that would come with CICE. Kind of like what is done for the shr_orb routines. When we use CICE in CESM, then CDEPS is part of the CESM checkout. One thing you might be able to do is have CDEPS as a submodule / external for CICE. This is only checked out when using UFS. Perhaps I am missing something still? |
|
Sorry for being confusing. UFS already has CDEPS as a subcomponent. This code builds and runs fine in UFS (with appropriate linking). It shouldn't change anything for CESM either. I was worried if anyone else uses this driver and does not have CDEPS. If that is not a concern, I agree with @anton-seaice that this code is fine as is until further notice. No stubs (or CDEPS submodule) are needed for cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 then |
| use ESMF, only : ESMF_LogFoundError, ESMF_LOGERR_PASSTHRU, ESMF_Finalize, ESMF_END_ABORT | ||
|
|
||
| #ifndef CESMCOUPLED |
There was a problem hiding this comment.
I think we should leave this section, and add a UFSCOUPLED or a NO_CDEPS FLAG .
There was a problem hiding this comment.
It seems cleaner to admit the dependency on CDEPS. Is this ok @anton-seaice?
There was a problem hiding this comment.
Consensus seems to be to go with the PR as currently written
Sorry - you are right. I didn't realise most of the ice_prescribed_mod file was within the CESM coupled ifdef. I think we shouldn't do something that might require others to add a compiler flag if their existing setup works ... ? I added a comment to this effect - https://github.com/CICE-Consortium/CICE/pull/964/files#r1708080117 |
|
I like this strategy of adding a NOCDEPS flag or something similar. However, how many groups use the nuopc/cmeps driver? If it is only the three of us and we all use CDEPS, then I would advocate for removing the CESMCOUPLED flag here. |
I don't think any ifdef is required in the |
|
We should probably add an informative message here to let people know that CDEPS is required. |
I don't see any docs for drivers/nuopc/cmeps . May I ask where I should inform of this dependency on CDEPS? |
|
There is no documentation for the driver per se. I think we just need a graceful abort that checks the first call to CDEPS and prints a message saying that CDEPS is required for this module or something like that. |
|
Using this code as is without any "substitute" ifdefs, the nuopc/cmeps driver code should fail to compile without CDEPS. This isn't a practical problem as CDEPS comes with CMEPS in coupled models we know of. @anton-seaice, do y'all share this driver too? It is useful to enable this prescribed ice code more generally beyond CESMCOUPLED. Maybe the position here is: Why "clutter" the code with ifdefs, stub routines, etc. unless needed? Especially if just to abort. This is of course a Consortium decision and open to other solutions. |
|
Excellent point. So, I agree with removing the ifdefs and it is just buyer beware for those using the nuopc/cmeps driver. Maybe something in the comments about the dependency on CDEPS? That way if it fails to compile, someone could look in ice_prescribed_mod.F90 and see this note. |
|
My feeling is that if the nuopc/cmeps implementation is always used with cdeps as far as we know, then get rid of the CPPs. Especially if they are not tested. If someone comes along in the future that uses nuopc/cmeps and doesn't have cdeps (and doesn't need prescribed ice mode), they can add the CPPs and test it at that time. |
|
Is this ready to merge? @dabail10, can you review/confirm when you have a chance. It's fine by me but have no stake in and have not tested these changes. Thanks. |
|
I have not had time to test this. I think it is fine. |
|
@dabail10. I'll wait for you to approve. I'm not in a rush. Will leave it up to you whether to test or not before merging. |
|
Just tested this in CESM and it passed a simple debug restart test. |
Prescribed ice CICE was enclosed by #ifdef CESMCOUPLED in CMEPS driver. This allows use of prescribed ice CICE in ufs-weather-model with CDEPS calls to input (sea ice concentration) from file. Starting as cherry-picked as in UFS but dependency on CDEPS likely needs to be optional at compile time. This is needed for History conflicts between CICE-Consortium and emc/develop #84 * Enable prescribed ice for UFS (#80) * Remove #ifdef CESMCOUPLED from cicecore/drivers/nuopc/cmeps/CICE_RunMod.F90 * Remove #ifdef CESMCOUPLED from cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 * Testing changing stream_taxmode from cycle to extend * Change include mpif.h to use mpi in ice_prescribed_mod.F90 * Call dshr_pio_init from ice_prescribed_init * Call ice_prescribed_run after ice_prescribed_init * Incorporate ice_prescribed_nml in ice_prescribed_mod.F90 * write stream_taxmode to log. typo in ice_prescribed_mod.F90. * Remove CESMCOUPLED so models initialize ice_prescribed at end of ice_prescribed_init * Change indent for #ifndef for gnu debug * Only call ice_prescribed_run in ice_prescribed_init #ifndef CESMCOUPLED * Note CDEPS dependency in cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 * Remove stub prescribed ice code
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
Enable prescribed ice CICE for UFS in nuopc/cmeps cap
Nicholas Szapiro & Denise Worthen
Regression test atm_ds2s_docn_pcice added to ufs-weather-model (Add active atmosphere applications with data ocn and data ice components (ATM_DS2S and ATM_DS2S-PCICE) and regression tests (atm_ds2s_docn_dice and atm_ds2s_docn_pcice) ufs-community/ufs-weather-model#2186)
Prescribed ice CICE was enclosed by #ifdef CESMCOUPLED in CMEPS driver. This allows use of prescribed ice CICE in ufs-weather-model with CDEPS calls to input (sea ice concentration) from file. Starting as cherry-picked as in UFS but dependency on CDEPS likely needs to be optional at compile time. This is needed for History conflicts between CICE-Consortium and emc/develop NOAA-EMC/CICE#84