Skip to content

Support for HAFS UFSATM-HYCOM and DATM-HYCOM coupling.#154

Merged
uturuncoglu merged 19 commits into
ESCOMP:masterfrom
hafs-community:support/HAFS
Jan 28, 2021
Merged

Support for HAFS UFSATM-HYCOM and DATM-HYCOM coupling.#154
uturuncoglu merged 19 commits into
ESCOMP:masterfrom
hafs-community:support/HAFS

Conversation

@danrosen25
Copy link
Copy Markdown
Collaborator

@danrosen25 danrosen25 commented Jan 21, 2021

Description of changes

Added mapping type for UFSATM-HYCOM coupling. New mapping type fills in destination points with 9.99e20, a physically unrealistic value, before executing bilinear regridding. Then bilinear regridding does not modify unmapped points. This allows destination component to identify unmapped points.

Small fixes were made to check FieldBundleIsCreated before accessing data pointer from FB. I believe this is due to a domain with a sparse decomposition.

HAFS field coupling configuration was modified/added to esmFldsExchange_hafs_mod.F90. There are now two configurations. system_type=CDEPS configures DATM-Active_Ocean coupling and includes flux calculation in the mediator. system_type=UFS configures Active_Atmosphere-Active_Ocean coupling and does not compute fluxes. I do have an Issue opened for this topic but the issue is only temporarily addressed here by adding the system_type to esmFldsExchange_hafs_mod.F90.
#92

This PR also aims to remove internal PIO from CMEPS completely and move the shared code to another folder (we removed the nems from the directory structure). The PIO initialization is in the NEMS side. @DeniseWorthen you need to use following branch in the PR to work under S2S (hafs-community/NEMS#2) because NEMS has additional mods that is required for PIO initialization.

Specific notes

Contributors other than yourself, if any:
@uturuncoglu - Worked on the DATM-Active_Ocean coupling for HAFS including internal PIO removal
@danrosen25 - Worked on the Active_Atmosphere-Active_Ocean coupling for HAFS including new mapping type

CMEPS Issues Fixed (include github issue #):
Addressed this issue by creating 'mapfillv_bilnr'
#91

Are changes expected to change answers?

  • bit for bit
  • different at roundoff level
  • more substantial

Any User Interface Changes (namelist or namelist defaults changes)?

  • Yes
  • No

Testing performed if application target is CESM:(either UFS-S2S or CESM testing is required):

  • (recommended) CIME_DRIVER=nuopc scripts_regression_tests.py
  • (recommended) CESM testlist_drv.xml
    • machines and compilers: Cheyenne, Intel 19.0.5 and MPT 2.15
    • details (e.g. failed tests): There are no failed tests expect following computation and memory increases
     FAIL ERS_Vnuopc_Ln9_N3.f19_g17_rx1.A.cheyenne_intel TPUTCOMP Error: TPUTCOMP: Computation time increase > 25% from baseline
     FAIL SMS_Vnuopc_Ld1_N3.f19_g17_rx1.A.cheyenne_intel TPUTCOMP Error: TPUTCOMP: Computation time increase > 25% from baseline
     FAIL SMS_Vnuopc_Ld5.T62_t061.GMOM.cheyenne_intel MEMCOMP Error: Memory usage increase > 10% from baseline
    
  • (optional) CESM prealpha test
    • machines and compilers
    • details (e.g. failed tests):
  • (other) please described in detail
    • machines and compilers
    • details (e.g. failed tests):

Testing performed if application target is UFS-coupled:

  • [ x] (recommended) UFS-coupled testing
    • description: all coupled and datm tests pass
    • details (e.g. failed tests):

Testing performed if application target is UFS-HAFS:

  • (recommended) UFS-HAFS testing
    • description: Testing conducted with UFSATM-HYCOM coupling and DATM-HYCOM coupling on Hera and Orion
    • details (e.g. failed tests): Testing will not produce bit-for-bit results when compared to UFSATM-HYCOM coupled through NUOPC Connector. This is expected due to term ordering differences in the SMM factor generation for a grid converted to a mesh. ESMF has a branch of work that can be used to synchronize the term ordering https://github.com/esmf-org/esmf/tree/gtom_bfb_debug

Hashes used for testing:

@danrosen25
Copy link
Copy Markdown
Collaborator Author

@uturuncoglu
Build failed due to the internal PIO removal. Does the build-cmeps test script need to be updated to account for the PIO change?

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@danrosen25 if you are talking about the GitHub action failure this is expected. We need to fix it. @jedwards4b do you want me to look at?

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@mvertens @DeniseWorthen I'll test this branch with CESM and add the details to PR.

@mvertens
Copy link
Copy Markdown
Collaborator

@uturuncoglu - thank you for offering to test with CESM. It was not clear to me from the changes in med_map_mod that this would not effect the cesm results - or the UFS results.

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@mvertens the changes in med_map_mod probably does not affect other applications because @danrosen25 introduced a new interpolation and I put some fix for the masking in the data component and they are isolated for HAFS app. Eventually, the definition of source and destination mask values need to be revisited to make this part of code more clean in CMEPS side. Currently, every application (or coupling mode) has its own way. Anyway, let me run the full CESM test suite and see what happens. If we see any answer change or etc. we could log at more detailed. Which baseline do I need to use for it?

@mvertens
Copy link
Copy Markdown
Collaborator

mvertens commented Jan 21, 2021 via email

@DeniseWorthen
Copy link
Copy Markdown
Collaborator

DeniseWorthen commented Jan 21, 2021

I'm not able to build this in ufs-weather; I'm getting a error :

-- Configuring done
CMake Error at CMEPS-interface/CMakeLists.txt:73 (add_library):
  Cannot find source file:

    CMEPS/nems/util/shr_abort_mod.F90

  Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++ .hm
  .hpp .hxx .in .txx


CMake Error at CMEPS-interface/CMakeLists.txt:73 (add_library):
  No SOURCES given to target: cmeps


CMake Generate step failed.  Build files cannot be regenerated correctly.

I cloned my current ufs-weather fork and pointed to the support/HAFS branch ( /glade/work/worthen/ufs_testhafs).

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@DeniseWorthen I think you also need to update UFS model itself, at least the CMEPS-interface/CMakeLists.txt file to test with S2S. Could you replace your CMEPS-interface/CMakeLists.txt file with the following and try again,

https://github.com/hafs-community/ufs-weather-model/blob/feature/hafs_couplehycom/CMEPS-interface/CMakeLists.txt

Eventually, all those model level changes need to be synced. Let me know if you have any other problem.

@danrosen25
Copy link
Copy Markdown
Collaborator Author

Hi @DeniseWorthen

The files in CMEPS/nems/util/ were moved a823ee5

You'll have to update the _nems_util_files list in CMEPS-interface/CMakeLists.txt

list(APPEND _nems_util_files
CMEPS/util/shr_abort_mod.F90
CMEPS/util/shr_log_mod.F90
CMEPS/util/shr_pio_mod.F90
CMEPS/util/shr_sys_mod.F90
CMEPS/util/shr_flux_mod.F90
CMEPS/util/shr_mpi_mod.F90
CMEPS/util/glc_elevclass_mod.F90
CMEPS/util/shr_mem_mod.F90
CMEPS/util/shr_kind_mod.F90
CMEPS/util/perf_mod.F90
CMEPS/util/shr_const_mod.F90)

@jedwards4b
Copy link
Copy Markdown
Collaborator

@danrosen25 If you merge the latest master to this PR the extbuild should now pass.

@danrosen25
Copy link
Copy Markdown
Collaborator Author

@jedwards4b Is it okay to remove the empty .gitmodules file?

@jedwards4b
Copy link
Copy Markdown
Collaborator

yes, thanks

@danrosen25
Copy link
Copy Markdown
Collaborator Author

@mvertens I submitted an issue for the hard coded masking by coupling_mode back in August #93

@DeniseWorthen
Copy link
Copy Markdown
Collaborator

@uturuncoglu @danrosen25 I've gotten it to build now in ufs-weather. I'll run the ufs-coupled and datm tests.

Comment thread mediator/med.F90 Outdated
Comment thread mediator/med_map_mod.F90 Outdated
Comment thread mediator/med_methods_mod.F90 Outdated
@mvertens
Copy link
Copy Markdown
Collaborator

mvertens commented Jan 25, 2021 via email

@jedwards4b
Copy link
Copy Markdown
Collaborator

These tests use master

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@mvertens yes, I am using CDEPS master. BTW, is PET_Ln9_P4.f19_f19.A.cheyenne_intel.fake_testing_only test same with PET_Ln9_P4.f19_f19.A.cheyenne_intel. I am asking because I could not create a test using ./create_test PET_Ln9_P4.f19_f19.A.cheyenne_intel.fake_testing_onlybut ./create_test PET_Ln9_P4.f19_f19.A.cheyenne_intel is working. @jedwards4b I did not downgrade CDEPS. BTW, PET_Ln9_P4.f19_f19.A.cheyenne_intel is passing with my PR also. So, I am not sure that I am testing same test that was failing. @jedwards4b let me know what do you think?

@jedwards4b
Copy link
Copy Markdown
Collaborator

you need to add Vnuopc to your test to test CDEPS

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@jedwards4b okay, let me try again. Thanks.

@mvertens
Copy link
Copy Markdown
Collaborator

mvertens commented Jan 25, 2021 via email

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@mvertens @jedwards4b thanks for quick fix. I am testing it again.

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@mvertens @jedwards4b all the tests are passed after recent fix that Mariana put in CDEPS side. You could see the full log in the following path /glade/u/home/turuncu/NCAR/CESM_test/cime/scripts/tests/log3

@jedwards4b
Copy link
Copy Markdown
Collaborator

jedwards4b commented Jan 25, 2021

You still have a fail in the github build. It's a real error that needs to be fixed:
/home/runner/work/CMEPS/CMEPS/mediator/esmFldsExchange_hafs_mod.F90:944:38: (all of these strings need to have the same length - pad them all to 4)

   specialStringList=(/"off","low","high","max"/), &

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@jedwards4b yes, I know. I'll look at it. CESM tests and regression tests are compiled without any problem. I think it is related with GNU. I'll fix it soon.

@uturuncoglu
Copy link
Copy Markdown
Collaborator

@jedwards4b let's see what happens. I hope it will pass this time.

coupling_mode. Check pointers returned from fldbun_getdata1d calls.
Remove zero size allocation from med_methods_FB_reset.
@uturuncoglu
Copy link
Copy Markdown
Collaborator

@danrosen25 I need to check this version of PR with data components especially modifications related with the masking. I'll let you know about it.

@danrosen25
Copy link
Copy Markdown
Collaborator Author

Hi @jedwards4b @DeniseWorthen
I believe I addressed all the code review comments left in the latest commit.
ab3fd30

I apologize for the string array mismatching length issue. That is something that depends on the compiler and I didn't notice it.

@DeniseWorthen
Copy link
Copy Markdown
Collaborator

I will run a sub-set of our tests just to confirm. I don't expect any negative impacts. Thanks.

Copy link
Copy Markdown
Collaborator

@DeniseWorthen DeniseWorthen left a comment

Choose a reason for hiding this comment

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

Looks good.

Comment thread mediator/med.F90
Comment thread mediator/med.F90 Outdated
Comment thread mediator/med.F90 Outdated
Comment thread mediator/med_map_mod.F90 Outdated
@uturuncoglu
Copy link
Copy Markdown
Collaborator

@danrosen25 i tested this version of brach and it works without any problem in the data configurations.

@mvertens
Copy link
Copy Markdown
Collaborator

mvertens commented Jan 27, 2021 via email

@danrosen25
Copy link
Copy Markdown
Collaborator Author

I apologize for another change. I reviewed the PR code changes one more time and found another allocate size zero. I also modified the previous allocate size zero fix to match the logic used for this one for consistency. It won't have an effect on any of the previous runs.
19ff711

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.

New Mapping Type - Bilinear with Fill Values

5 participants