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

Passing additional surface variables into the atmospheric model #28 #29

Merged
merged 8 commits into from
Nov 27, 2023

Conversation

zhihong-tan
Copy link
Contributor

Description
This PR is linked to issue #28 and it depends on issue #92 (#93) of the FMScoupler repository.

A new boundary layer scheme (NCEP TKE-based eddy-diffusivity mass-flux scheme) is being implemented in the prototype-AM5 model, and it requires five additional surface variables (shflx, lhflx, wind, thv_atm, and thv_surf) as inputs. Thus, I would like to add a few lines in the atmos_driver (coupled/atmos_model.F90) to pass these variables into the physics driver. (A "ifdef" statement is added to maintain backward compatibility with AM4's physics driver).

How Has This Been Tested?
The AM4 8-day 'basic' run with my updated FMScoupler and atmos_drivers can bitwise reproduce the results with those run with the default code. This test is done with the AM4-2023.02 xml obtained with the command below:

git clone -b AM4_2023.02 http://gitlab.gfdl.noaa.gov/mdt/xml.git xml_2023.02

And the model codes are compiled and run on the ncrc5.intel22-classic-prod-openmp platform.

Note that both changes in FMScoupler and atmos_drivers need to be incorporated at the same time for the model to compile and run correctly.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [n/a] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • [n/a] New check tests, if applicable, are included

Zhihong Tan added 3 commits October 26, 2023 19:09
The shflx, lhflx, wind, thv_atm, thv_surf variables are passed
from the surface coupler to the atmospheric model as required by
the NCEP TKE-EDMF scheme.
For compatibility with AM4, a ifdef-statement with a conditional
compilation flag "do_am5phys" is added in the call to
physics_driver_down to prevent the additional surface variables
from being passed into the old AM4 physics package. This flag
should be defined if AM5 is compiled with NCEP TKE-based EDMF.
@thomas-robinson
Copy link
Member

Does the solo driver also need to be updated?

@laurenchilutti
Copy link
Contributor

@menzel-gfdl Would you have the time to review this today?

@zhihong-tan
Copy link
Contributor Author

zhihong-tan commented Nov 9, 2023 via email

Zhihong Tan and others added 3 commits November 10, 2023 11:05
The added variables are now passed as optional arguments into
physics_driver_down. This requires consistent changes in the
atmos_phys package.
Two if-associated statements are added before the checksums
of bnd_type%shflx and bnd_type%lhflx to pass the null model
build.
@laurenchilutti
Copy link
Contributor

@zhihong-tan I see you made a few commits - are these changes ready to review? And has the atmos_phys PR been created?

@zhihong-tan
Copy link
Contributor Author

zhihong-tan commented Nov 13, 2023

@laurenchilutti I have completed and committed my code changes, but I would need to go through again a couple of verification tests before submitting the merge request (I will do this today). And I have created my own forks of atmos_phys on both Github and Gitlab.

@zhihong-tan
Copy link
Contributor Author

zhihong-tan commented Nov 13, 2023

@laurenchilutti I have created the PRs and merge requests on Gitlab on atmos_solo_land and atmos_phys repositories. Especially, the update on atmos_phys is needed along with the updates on FMScoupler and atmos_drivers for the standard AM4 to run successfully.

Could you please go over these PR/MRs for me? Also, could you please suggest who I should assign the MRs to (on Gitlab)?Thank you!

@zhihong-tan
Copy link
Contributor Author

zhihong-tan commented Nov 13, 2023

Additionally, I have also created my own fork of the atmos_phys repository on Github and made the same changes as those on Gitlab on my branch. However, because Github's atmos_phys has not been updated since 2021.02, I have not been successful in creating a working version of AM4 with the up-to-date repositories of other components (e.g., FMS and FMScoupler) on Github. Should I still submit the PR on Github?

@uramirez8707
Copy link

Also, could you please suggest who I should assign the MRs to (on Gitlab)?Thank you!

I took care of the atmos_phys PR and tagged it as 2023.04-beta1 (It will be tagged as 2023.04 once we run regression testing on it)

Additionally, I have also created my own fork of the atmos_phys repository on Github and made the same changes as those on Gitlab on my branch. However, because Github's atmos_phys has not been updated since 2021.02, I have not been successful in creating a working version of AM4 with the up-to-date repositories of other components (e.g., FMS and FMScoupler) on Github. Should I still submit the PR on Github?

The github version of atmos_phys is only updated as needed for publications, so it does not need to be updated in this case. Thanks

@laurenchilutti
Copy link
Contributor

@thomas-robinson and @menzel-gfdl when you get a moment could you please review this PR this is associated with changes in atmos_phys which you can look at Gitlab for.

coupled/atmos_model.F90 Outdated Show resolved Hide resolved
Comment on lines +1429 to +1431
write(outunit,100) 'lnd_ice_atm_bnd_type%wind ',mpp_chksum(bnd_type%wind )
write(outunit,100) 'lnd_ice_atm_bnd_type%thv_atm ',mpp_chksum(bnd_type%thv_atm )
write(outunit,100) 'lnd_ice_atm_bnd_type%thv_surf ',mpp_chksum(bnd_type%thv_surf )
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking these entities will be defined for all non-AM5 simulations as well e.g. AM3/AM4.

The full name for the NCEP TKE-based EDMF replaces the informal
short name (HanEDMF) in the comments only. No other code changes.
@laurenchilutti
Copy link
Contributor

@thomas-robinson @menzel-gfdl would you be able to review this today?

The term 'surface' in the comments for the added fields is
clarified as the lowest atmos level as Sergey suggested.
@laurenchilutti
Copy link
Contributor

@thomas-robinson can you give this a final review and merge if approved?

@laurenchilutti laurenchilutti merged commit 62279bb into NOAA-GFDL:main Nov 27, 2023
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