ufs-community / ufs-weather-model

UFS Weather Model
Other
130 stars 238 forks source link

Merge latest RUC LSM into community develop #1646

Closed tanyasmirnova closed 1 year ago

tanyasmirnova commented 1 year ago

Description

This PR merges changes from RRFS_dev to UFS community develop related to:

  1. Introducing new variables for fractional vegetation and soil data and and read this data in FV3GFS_io.F90.
  2. Updated snow model in RUC LSM
  3. Changes in the output related to LSM-produced variables.<!--

Provide a detailed description of what this PR does. What bug does it fix, or what feature does it add? Is a change of answers expected from this PR? Are any library updates included in this PR (modulefiles etc.)? -->

Top of commit queue on: TBD

Input data additions/changes

Anticipated changes to regression tests:

Subcomponents involved:

Combined with PR's (If Applicable):

Commit Queue Checklist:

Linked PR's and Issues:

Testing Day Checklist:

Testing Log (for CM's):

BrianCurtis-NOAA commented 1 year ago

I see you put Draft: into the title. Thank you for that. If you want to make it a draft in GitHub, you can convert it to draft by clicking the link in the Reviewers section on the right side, you should see something like: Still in progress? Convert to draft. Click Convert to draft.

grantfirl commented 1 year ago

@tanyasmirnova Have these changes been tested yet? If not, do you need some assistance?

tanyasmirnova commented 1 year ago

@grantfirl Thank you for your question. I am still addressing your comments and the code is still changing. However @RatkoVasic-NOAA started performing a regression tests for my PR. We had a show stopper with compiling -DCCPP_SUITES=FV3_GFS_v17_coupled_p8_sfcocn -DCMEPS_AOFLUX=ON. It appears that CMEPS_AOFLUX has its own meta file that needs to be updated. This is where we are right now.

uturuncoglu commented 1 year ago

@tanyasmirnova CMEPS is also defined as CCPP host model for calculating atmosphere-ocean fluxes on exchange grid. So, it has own data structures and metadata file. Let me know if you need help.

tanyasmirnova commented 1 year ago

@uturuncoglu I am adding some variables to MED_typedefs.meta. I have a questions: what section should I put xlat_d, xlon_d. They are defined under Grid property in GFS_typedefs.meta: allocate (Grid%xlat_d (IM)) allocate (Grid%xlon_d (IM))

tanyasmirnova commented 1 year ago

@uturuncoglu I think I figured it out.

tanyasmirnova commented 1 year ago

@grantfirl @uturuncoglu When I added missing variables to CMEPS metadata, I got the following error in compilation with CMEPS:Compiling CMEPS with CCPP support. Calling CCPP code generator (ccpp_prebuild.py) for suites --suites=FV3_sfc_ocean ... -- CCPP prebuild step completed successfully -- Configuring done CMake Error at MOM6-interface/CMakeLists.txt:28 (add_library): Cannot find source file:

MOM6/src/equation_of_state/TEOS10/gsw_chem_potential_water_t_exact.f90

Tried extensions .c .C .c++ .cc .cpp .cxx .cu .mpp .m .M .mm .h .hh .h++ .hm .hpp .hxx .in .txx .f .F .for .f77 .f90 .f95 .f03 .ispc

CMake Error at CICE-interface/CMakeLists.txt:71 (add_library): Cannot find source file:

CICE/icepack/columnphysics/icepack_aerosol.F90

The log file is in /scratch1/BMC/gsd-fv3-dev/smirnova/ccpp_fv3/ufs_community_6mar23/tests/comp_CMEPS.log

Could you suggest us how to fix this error?

uturuncoglu commented 1 year ago

@uturuncoglu I suggest to have clean build by changing "${BUILD_JOBS:-4}" parameter in ./build.sh (in the root model directory) and make it run with 1 thread like "${BUILD_JOBS:-1}". So, this will allow you to compile model component sequentially and make the error more apparent. Form your error output, it seems that the problem is on other components. BTW, I could only access to Orion and Cheyenne. So, if you want me to look at we need to compile on those platforms.

RatkoVasic-NOAA commented 1 year ago

@uturuncoglu Ufuk, I updated Tanya's branch so you can try to compile on Orion or Cheyenne. Here is the procedure:

git clone -b ufsdev_ruclsm https://github.com/tanyasmirnova/ufs-weather-model.git
cd ufs-weather-model
git submodule update --init --recursive

ls -l CMEPS-interface/CMEPS/ufs/ccpp/data/  <---- changes are here

cd tests
compile.sh @machine.compiler@ "-DAPP=S2S -DCCPP_SUITES=FV3_GFS_v17_coupled_p8_sfcocn -DCMEPS_AOFLUX=ON"
grantfirl commented 1 year ago

@tanyasmirnova @RatkoVasic-NOAA @uturuncoglu @DeniseWorthen It was pointed out in the UFS code management meeting that we'll need a PR in CMEPS for the changes there that should be linked to this PR.

ufsdev_ruclsm->emc/develop

tanyasmirnova commented 1 year ago

Thank you very much, Everybody, for your help. Ratko, could you please make a PR in CMEPS?

RatkoVasic-NOAA commented 1 year ago

Thank you very much, Everybody, for your help. Ratko, could you please make a PR in CMEPS?

I can do the PR, but compilation is still failing.

uturuncoglu commented 1 year ago

@tanyasmirnova @RatkoVasic-NOAA I ll look at this today. I am out of office now.

RatkoVasic-NOAA commented 1 year ago

@tanyasmirnova @RatkoVasic-NOAA @uturuncoglu @DeniseWorthen It was pointed out in the UFS code management meeting that we'll need a PR in CMEPS for the changes there that should be linked to this PR.

ufsdev_ruclsm->emc/develop

PR https://github.com/NOAA-EMC/CMEPS/pull/84 is in.

tanyasmirnova commented 1 year ago

@RatkoVasic-NOAA @uturuncoglu @SamuelTrahanNOAA Sam is on board to help with testing of this PR. The SD with CMEPS is still not compiling.

uturuncoglu commented 1 year ago

@tanyasmirnova @RatkoVasic-NOAA I think I fixed the issue. The model was giving error like following,

/glade/scratch/turuncu/ufs-weather-model/tests/build_fv3_test/CMEPS-interface/physics/ccpp_static_api_med.F90(42): error #6580: Name in only-list does not exist or is not accessible.   [CON_KARMAN]
   use MED_typedefs, only: con_karman
---------------------------^
/glade/scratch/turuncu/ufs-weather-model/tests/build_fv3_test/CMEPS-interface/physics/ccpp_static_api_med.F90(198): error #6404: This name does not have a type, and must have an explicit type.   [CON_KARMAN]
                  con_rd=con_rd,con_rocp=con_rocp,con_karman=con_karman)
-------------------------------------------------------------^

and the karman constant is defined as following in the FV3/ccpp/physics/physics/physcons.F90

real(kind=kind_phys),parameter:: karman     =0.4_kind_phys                       !< Von Karman constant

so, it could not find con_karman since it is not in the physcons and it is not included with use statement. It think you need to do following change in the CMEPS/ufs/ccpp/data/MED_typedefs.F90.

+++ b/ufs/ccpp/data/MED_typedefs.F90
@@ -7,6 +7,7 @@ module MED_typedefs
   use physcons, only: con_hvap, con_cp, con_rd, con_eps, con_rocp
   use physcons, only: con_epsm1, con_fvirt, con_g 
   use physcons, only: con_tice
+  use physcons, only: con_karman => karman

   implicit none

This compiles without any issue in my side.

SamuelTrahanNOAA commented 1 year ago

Actually, @RatkoVasic-NOAA already fixed it by changing the name to karman in the MED_typedefs.meta and using the karman from physcons. It compiles for both @tanyasmirnova and I now.

uturuncoglu commented 1 year ago

@SamuelTrahanNOAA Thanks. I did not aware of it. Anyway, double fix is always better then the single one.

RatkoVasic-NOAA commented 1 year ago

Actually, @RatkoVasic-NOAA already fixed it by changing the name to karman in the MED_typedefs.meta and using the karman from physcons. It compiles for both @tanyasmirnova and I now.

That was @tanyasmirnova change, I just submitted it :-)

SamuelTrahanNOAA commented 1 year ago

Tanya has been diligintly working on getting the regression tests to pass, but we're left with these six that change results for unknown reasons:

SamuelTrahanNOAA commented 1 year ago

I have independently tested the restart bug fix, and the only test whose value changes is rrfs_smoke_conus13km_hrrr_warm. That means the hafs test changes can't be blamed on the restart fix.

jkbk2004 commented 1 year ago

@SamuelTrahanNOAA sounds a bit more time is needed to figure out those hafs cases, right?

SamuelTrahanNOAA commented 1 year ago

I think the HAFS cases are due to bug fixes like correcting metadata and real kinds, but we're still trying to pinpoint exactly what change caused it. There are now only five unaccounted for:

jkbk2004 commented 1 year ago

@SamuelTrahanNOAA no need to rush. do you think we can let #1672 (easy case with no baseline change and just CICE update) go first today?

SamuelTrahanNOAA commented 1 year ago

No need to rush. do you think we can let https://github.com/ufs-community/ufs-weather-model/pull/1672 (easy case with no baseline change and just CICE update) go first today?

There is a need to rush, since this has to be merged by the end of the week. If you think there's enough time to merge both, then yes, go ahead.

jkbk2004 commented 1 year ago

No need to rush. do you think we can let #1672 (easy case with no baseline change and just CICE update) go first today?

There is a need to rush, since this has to be merged by the end of the week. If you think there's enough time to merge both, then yes, go ahead.

Sure! we should be ok with timeline.

SamuelTrahanNOAA commented 1 year ago

I isolated the HAFS differences down to these:

  1. Change from real to real(kind_phys) in namelist_soilveg_ruc.F90 and set_soilveg_ruc.F90. HAFS uses these variables even though it is not running RUC LSM. Changing the HAFS results is expected here.
  2. Move four fields from CCPP_typedefs to GFS_typedefs so they can be output. They're now zeroed every FHZERO hours. This changes the results of the moving nest only. This may be changing due to unrelated issues in the moving nest implementation.

I'd like to hear from the HAFS developers on this: @BinLiu-NOAA @BijuThomas-NOAA

SamuelTrahanNOAA commented 1 year ago

@tanyasmirnova - github has made a complete wreck of your post. Could you please push the changes instead?

tanyasmirnova commented 1 year ago

Some variables in /scratch1/NCEPDEV/stmp2/Samuel.Trahan/FV3_RT/rt_143256/hafs_regional_specified_moving_1nest_atm look very noisy. For example: Screenshot 2023-03-28 at 3 52 23 PM Screenshot 2023-03-28 at 3 51 48 PM

SamuelTrahanNOAA commented 1 year ago

The baseline also has this problem. Here are plots of this:

ncview /scratch1/NCEPDEV/nems/emc.nemspara/RT/NEMSfv3gfs/develop-20230324/INTEL/hafs_regional_specified_moving_1nest_atm/sfc.nest02.f006.nc

transpiration-baseline pcloud

SamuelTrahanNOAA commented 1 year ago

We can proceed with this pull request. It does not cause the HAFS nest motion issue; that issue is already present in the baseline.

SamuelTrahanNOAA commented 1 year ago

My previous comment had an error. The four fields that are moved are still instantaneous. All that is changed is where they are stored.

BinLiu-NOAA commented 1 year ago

In a separate email thread discussion among @SamuelTrahanNOAA, @tanyasmirnova, @wramstrom and @BinLiu-NOAA, we confirm that the temporal statistical (average, minimum, maximum, accumulate, etc.) variables are known to not function properly for the moving nest domain. This is currently known and expected. Thanks!

grantfirl commented 1 year ago

FYI, the RT logs for the MYNN SFC PR before combining can be found here: https://github.com/ufs-community/ccpp-physics/pull/58

zach1221 commented 1 year ago

@SamuelTrahanNOAA do you still want to combine #1660 and #1678 into this PR?

SamuelTrahanNOAA commented 1 year ago

do you still want to combine https://github.com/ufs-community/ufs-weather-model/pull/1660 and https://github.com/ufs-community/ufs-weather-model/pull/1678 into this PR?

Both #1660 and #1678 are in here, plus one bug fix to #1660 that isn't in that PR. (Missing num in a !$omp clause.) I just updated this to the top of develop.

SamuelTrahanNOAA commented 1 year ago

@jkbk2004 - Please proceed with testing. I am confident that the combination of #1646 + #1660 + #1678 works. I have not tested them with the top of today's develop yet. That is unlikely to break this PR because today's develop only changed CICE. Nothing in this PR involves CICE.

zach1221 commented 1 year ago

@SamuelTrahanNOAA I see the new input data box is checked. If there is new input data we will need to re-sync it first, before testing. Can you tell us where the new data is?

SamuelTrahanNOAA commented 1 year ago

I see the new input data box is checked

@tanyasmirnova @joeolson42 - Is there new input data for these PRs?

I'm 99% certain that the "new input data" box was checked by accident. Can you confirm this?

SamuelTrahanNOAA commented 1 year ago

Both #1678 and #1660 say they do not need new input data. That means we just need a confirmation from @tanyasmirnova that this PR also does not need new input data.

tanyasmirnova commented 1 year ago

@SamuelTrahanNOAA @joeolson42 Actually, the change in FV3GFS_io.F90 is reading in extra variables that were added to the oro* files: fractional soil and fractional vegetation. This data is optional and the code should work even when it is not available (similar to lake fraction and lake depth).

SamuelTrahanNOAA commented 1 year ago

We've run the tests (several times) without that new data. We don't even know if the regression tests will work with the new data. Are you okay with merging this PR without the new data?

zach1221 commented 1 year ago

@BrianCurtis-NOAA @DeniseWorthen fyi we're going to start testing this PR next.

SamuelTrahanNOAA commented 1 year ago

@tanyasmirnova - Instead of delaying this PR, I suggest we put the new data in a later PR. We're planning to make a new 13km RRFS test case with correct lake data. When we do that, we'll add the fractional information to that test. For now, we'll leave the fractional data absent.

How does that sound?

tanyasmirnova commented 1 year ago

Yes, I am ok with merging this PR without a new data. This data is optional, and so far it is not used. But the code is in place to read it when it is available.

zach1221 commented 1 year ago

Ok, I'll mark the "No changes are expected to input data." box.

tanyasmirnova commented 1 year ago

Sounds good. We can make the PR for additional data later.

zach1221 commented 1 year ago

Jenkins-ci logs attached. ORTs against this PR have passed. ufs-weather-model » ort-docker-pipeline » PR-1646 #1 Console [Jenkins].pdf

jkbk2004 commented 1 year ago

Automated RT Failure Notification Machine: orion Compiler: intel Job: BL [BL] Repo location: /work/noaa/epic-ps/jongkim/autort/pr/1268571848/20230329183016/ufs-weather-model Please make changes and add the following label back: orion-intel-BL

zach1221 commented 1 year ago

Disregard the Orion error above, I think it's jenkins pipeline related.