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

Enable prescribed ice for UFS #964

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

NickSzapiro-NOAA
Copy link
Contributor

@NickSzapiro-NOAA NickSzapiro-NOAA commented Aug 2, 2024

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

  • Short (1 sentence) summary of your PR:
    Enable prescribed ice CICE for UFS in nuopc/cmeps cap
  • Developer(s):
    Nicholas Szapiro & Denise Worthen
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    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)
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit (when not in prescribed ice)
    • different at roundoff level
    • more substantial (when in prescribed ice #ifndef cesmcoupled)
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • 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.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.
    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

* 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
@NickSzapiro-NOAA
Copy link
Contributor Author

NickSzapiro-NOAA commented Aug 2, 2024

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 ?

Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine as far as I can tell. I will run with it on Monday to make sure.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These come from the CIME shr stuff I believe. This is how we do the reading from stream files.

@NickSzapiro-NOAA
Copy link
Contributor Author

NickSzapiro-NOAA commented Aug 2, 2024

Thanks Dave! So most of the code in cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90 was enclosed in an #ifndef CESMCOUPLED else:
https://github.com/CICE-Consortium/CICE/blob/main/cicecore/drivers/nuopc/cmeps/ice_prescribed_mod.F90#L30-L492

Currently, functions now "uncovered" would need these files for CDEPS inline functionality:
https://github.com/ufs-community/ufs-weather-model/pull/2186/files#diff-db20ac41c986cab4cffeeaf178a6a4abf1c092a137af4cbe274e19deb7326954R154-R171

One option is renaming the CESMCOUPLED here to something more general

@anton-seaice
Copy link
Contributor

If I am reading this correctly, there were already some depedencies on CDEPS (i.e. other use dshr_ statements) in this file and no-one has raised it as an issue? So I think we can just leave it be until someone wants to build without CDEPS? I have a vague recollection of someone saying there are other unexpected dependencies between CMEPS & CDEPS in places, so I don't think this would break anything particularly.

@dabail10
Copy link
Contributor

dabail10 commented Aug 7, 2024

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?

Comment on lines +210 to +213
#ifndef CESMCOUPLED
! If need initial cice values for coupling
call ice_prescribed_run(idate, msec)
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@NickSzapiro-NOAA
Copy link
Contributor Author

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 (?)

@dabail10
Copy link
Contributor

dabail10 commented Aug 7, 2024

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?

@NickSzapiro-NOAA
Copy link
Contributor Author

NickSzapiro-NOAA commented Aug 7, 2024

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave this section, and add a UFSCOUPLED or a NO_CDEPS FLAG .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems cleaner to admit the dependency on CDEPS. Is this ok @anton-seaice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consensus seems to be to go with the PR as currently written

@anton-seaice
Copy link
Contributor

anton-seaice commented Aug 7, 2024

There was only a dependency on CDEPS when CESMCOUPLED was defined. I guess my concern is that we have code duplication here.

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

@dabail10
Copy link
Contributor

dabail10 commented Aug 8, 2024

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.

@DeniseWorthen
Copy link
Contributor

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 ice_prescribed_mod at this point. If someone were to attempt to use this mode w/o CDEPS, they would probably either a) fail compile or b) fail at run time when the ice_prescribed_init routine tries to call the dshr_pio_init or shr_strdata_init_from_inline.

@dabail10
Copy link
Contributor

dabail10 commented Aug 8, 2024

We should probably add an informative message here to let people know that CDEPS is required.

@NickSzapiro-NOAA
Copy link
Contributor Author

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?

@dabail10
Copy link
Contributor

dabail10 commented Aug 8, 2024

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.

@NickSzapiro-NOAA
Copy link
Contributor Author

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.

@dabail10
Copy link
Contributor

dabail10 commented Aug 8, 2024

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.

@apcraig
Copy link
Contributor

apcraig commented Aug 8, 2024

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.

Copy link
Contributor

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this builds and starts, we don't use prescribed_ice so I didn't test that.

@apcraig
Copy link
Contributor

apcraig commented Aug 16, 2024

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.

@dabail10
Copy link
Contributor

I have not had time to test this. I think it is fine.

@apcraig
Copy link
Contributor

apcraig commented Aug 16, 2024

@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.

@dabail10
Copy link
Contributor

Just tested this in CESM and it passed a simple debug restart test.

@apcraig apcraig merged commit 635d9a1 into CICE-Consortium:main Aug 22, 2024
2 checks passed
@NickSzapiro-NOAA NickSzapiro-NOAA deleted the consortium_pcice branch September 24, 2024 17:01
NickSzapiro-NOAA added a commit to NOAA-EMC/CICE that referenced this pull request Sep 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants