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

Workaround to fix issues with wrong namelists for nests in fv3atm #86

Conversation

climbfuji
Copy link

@climbfuji climbfuji commented Mar 27, 2021

Description

This workaround is a temporary solution until the refactoring of the generic physics interface for calling GFDL/CCPP physics is completed. The issue is:

The current code in fv3atm has a bug when running nested models. The atmos_model_nml section, which contains options such as ccpp_suite, blocksize, fhdiag, is currently read in fv3atm before atmosphere_init is called. This is required, because atmosphere_init needs ccpp_suite.

However, the capability to read the correct namelist for the nests is only turned on during atmosphere_init. The obvious solution would be to move ccpp_suite into its own namelist section ccpp_nml, which could then be read by both the GFDL_atmos_cubed_sphere and fv3atm.

However: In light of the upcoming refactoring of the dycore and the ccpp-framework updates, this makes no sense. The planned implementation will use a generic interface in the dycore to either call the original GFDL fast physics (for SHIELD models etc) or the CCPP fast physics (for UFS etc), and will separate the existing suite definition files (SDFs) into two SDFs each (one for the dycore = fast physics, one for fv3atm = traditional physics). This means that we will have two ccpp_suite options in the namelist, one in the section relevant for the dycore (fv_core_nml), one in the section relevant for physics (currently gfs_physics_nml). It doesn't make sense to change each namelist template in the repository and instruct all developers to change their namelists twice - better do it once.

Therefore, the solution presented here (more workaround) is to read the namelist section atmos_model_nml in the dycore in the UFS-specific file driver/fvGFS/atmosphere.F90 to obtain the correct value of ccpp_suite after fv_control_init is called and before ccpp_physics_init is called.

For this to work, the reading of the namelist section atmos_model_nml must be moved to after atmosphere_init - see associated PR ufs-community/ufs-weather-model#488.

How Has This Been Tested?

I used the UFS regression test fv3_stretched_nest and updated input_nest02.nml to use a different suite definition file FV3_GFS_2017_stretched_nest (created manually just for testing) which is identical to the parent SDF except that it also calls h2ophysics. For consistency, I set h2o_phys = .true. in input_nest02.nml. I then added debug print statements in the CCPP code generator (ccpp_prebuild.py et al.) that told me that ranks 48+ (used for the nest) indeed called h2ophys, and that ranks 0-47 (used for the parent) did not.

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project - hope so
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - not yet
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules - n/a

Associated PRs:

#86
NOAA-EMC/fv3atm#263
ufs-community/ufs-weather-model#488

See ufs-community/ufs-weather-model#488 for testing.

XiaqiongZhou-NOAA and others added 2 commits January 25, 2021 16:09
…r height advection; change dz_min as a input (NOAA-GFDL#53)

* 1)Add an option to use original BC or zero-gradient BC to reconstruct interface u/v with PSM for height advection
2)Change dz_min as a namelist input
* Change to the original version by adjust the upper interface of ZH when dz<dz_min
* add authors for the revision
…on (section atmos_model_nml) in the input namelist
Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

I don't know what's going on here, but the diff seems off. The dz_min and psm_bc updates are already in NOAA-GFDL:dev/emc, but this comparison/diff makes it seem as if the baseline doesn't have them. I think you need to merge/rebaseline with NOAA-GFDL:dev/emc first?

@climbfuji
Copy link
Author

I don't know what's going on here, but the diff seems off. The dz_min and psm_bc updates are already in NOAA-GFDL:dev/emc, but this comparison/diff makes it seem as if the baseline doesn't have them. I think you need to merge/rebaseline with NOAA-GFDL:dev/emc first?

Yes, I need to rebase this first - it was made based on NOAA-EMC dev/emc last Friday.

@climbfuji
Copy link
Author

I don't know what's going on here, but the diff seems off. The dz_min and psm_bc updates are already in NOAA-GFDL:dev/emc, but this comparison/diff makes it seem as if the baseline doesn't have them. I think you need to merge/rebaseline with NOAA-GFDL:dev/emc first?

Yes, I need to rebase this first - it was made based on NOAA-EMC dev/emc last Friday.

Rebased, no more conflicts.

@bensonr
Copy link
Contributor

bensonr commented Mar 29, 2021

@climbfuji - can you create an issue to track cleaning this workaround up in a more elegant fashion.

@climbfuji
Copy link
Author

@climbfuji - can you create an issue to track cleaning this workaround up in a more elegant fashion.

Sure, please see here: #88

@climbfuji
Copy link
Author

@climbfuji - can you create an issue to track cleaning this workaround up in a more elegant fashion.

Sure, please see here: #88

How do we want to proceed with this PR?

@lharris4
Copy link
Contributor

I think this should be fine for now in the EMC branch. (In SHiELD we read this from atmos_model.F90.) It would be preferable if the code could all be put into a separate routine called from atmosphere_init() so that it is self contained but this isn't necessary right now.

@climbfuji
Copy link
Author

I think this should be fine for now in the EMC branch. (In SHiELD we read this from atmos_model.F90.) It would be preferable if the code could all be put into a separate routine called from atmosphere_init() so that it is self contained but this isn't necessary right now.

Thanks, Lucas. I'll work with Rusty to finalize this PR.

@climbfuji
Copy link
Author

@bensonr I got the blessing from Lucas for these changes, do you think we should proceed as is and fix issue #88 when we move to the generic physics interface?

@laurenchilutti
Copy link
Contributor

Once @junwang-noaa approves this I can go ahead and merge this into dev/emc

Copy link
Collaborator

@junwang-noaa junwang-noaa left a comment

Choose a reason for hiding this comment

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

I understand this fix is to allow the nest tasks to read in a different CCPP suite from the suite file read by other global domain tasks. Further work is required to have different settings for other variables in atmo_model_nml namelist.

@laurenchilutti laurenchilutti merged commit 6944f42 into NOAA-GFDL:dev/emc Apr 9, 2021
MicroTed pushed a commit to MicroTed/GFDL_atmos_cubed_sphere that referenced this pull request Sep 22, 2021
… model runs (NOAA-GFDL#86)

* make rain/snow tendency consistent with accumulated rain/snow

* put drain_cpl and dsnow_cpl in proper container

* Updates of IPD and CCPP code to regain bit-for-bit identical results for coupled model runs

* Update .gitmodules and submodule pointer for ccpp-physics for code review and testing

* gfsphysics/GFS_layer/GFS_physics_driver.F90: need to initialize local variables for bit-for-bit identical results

* Bugfix in gfsphysics/GFS_layer/GFS_typedefs.F90, allocate Tbd%drain_cpl and Tbd%dsnow_cpl when cplchm or cplflx is true

* Remove local/interstitial variables for seaice coupling, add suite definition file for S2S benchmark runs

* Rename S2S suite suite_FV3_GFS_2017_coupled_satmedmf.xml to suite_FV3_GFS_2017_satmedmf_coupled.xml

* Revert change to .gitmodules and update submodule pointer for ccpp-physics

Co-authored-by: Phil Pegion <[email protected]>
Co-authored-by: Philip Pegion <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants