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

Add import/export fields for fv3-jedi coupling #301

Merged
merged 10 commits into from
May 21, 2021

Conversation

DusanJovic-NOAA
Copy link
Collaborator

@DusanJovic-NOAA DusanJovic-NOAA commented May 19, 2021

Description

(Instructions: this, and all subsequent sections of text should be removed and filled in as appropriate.)
Provide a detailed description of what this PR does.
What bug does it fix, or what feature does it add?
This PR adds import and export fields to support fv3-jedi coupling
Is a change of answers expected from this PR?
No.

Testing

How were these changes tested? Full regression tests on Hera and WCOSS Dell.
What compilers / HPCs was it tested with? Intel
Are the changes covered by regression tests? Not every code change is covered by ufs-weather-model tests, we do not run jedi in any of regression tests.
Have the ufs-weather-model regression test been run? On what platform? Yes. Hera and Dell.

  • Will the code updates change regression test baseline? No.
  • Please commit the regression test log files in your ufs-weather-model branch.

Dependencies

If testing this branch requires non-default branches in other repositories, list them.
Those branches should have matching names (ideally)

Depends on ufs-community/ufs-weather-model/pull/586

Do PRs in upstream repositories need to be merged first? No.

@junwang-noaa
Copy link
Collaborator

@rsdunlapiv you mentioned about the ESMF_METHOD_INITIALIZE, would you please show us what the changes need to make in the SetServices in fv3_cap?
@rmontuoro Now we have two places to set up the coupled fields in fv3, assign_importdata/setup_exportdata and update_atmos_chemistry, I think the changes Dusan made for fve-judi should have no impact on fv3-chemistry coupling. Would you please double check? With the change, in your fv3-chem run, the shared 3D fields in fv3 export will be reset twice (in fv3 update_atmos_chemistry and then setup_exportdata), but should have no impact on fv3-chem run. The assign_importdata is only called by the namelist variable cplflx/cplwav, so it should have no impact either.

@DusanJovic-NOAA
Copy link
Collaborator Author

DusanJovic-NOAA commented May 19, 2021

@junwang-noaa thanks for noticing that assign_importdata is only called if one of the two cpl* flags is true. We need to change that to call it unconditionally, like we do with setup_exportdata.

@junwang-noaa
Copy link
Collaborator

@DusanJovic-NOAA I am not sure if it will work with fv3-chem if you remove that. @rmontuoro can confirm. I think it will break the fv3-chem with the current run sequence as those fv3 tracer fields (state t=n) should not be updated from the fv3 import state (state t=n-1 after chem) in the place where assign_importdata is called. More code changes are required in order to remove the cplflx/cplwav/cplchem.

@DusanJovic-NOAA
Copy link
Collaborator Author

we can then do this:

if( .not. GFS_control%cplchm ) then
  call assign_importdata(rc)
endif

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented May 19, 2021 via email

@@ -840,8 +826,8 @@ subroutine update_atmos_model_state (Atmos)
call atmosphere_get_bottom_layer (Atm_block, DYCORE_Data)

!if in coupled mode, set up coupled fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means for all runs, including APP=ATM atmosphere only runs, just not for coupled chemistry runs? Should the comments in lines 828 and 268 be updated?

@rsdunlapiv
Copy link

Per @junwang-noaa question about, this is a good time to clean up the SetServices to use ONLY the semantic labels for each of the phases. This is more clear than the "IPDv01p1" labels, which are hard to understand.

This means:
You don't need to register phase 0:

call ESMF_GridCompSetEntryPoint(gcomp, ESMF_METHOD_INITIALIZE,   userRoutine=InitializeP0, phase=0, rc=rc)

You don't need the subroutine InitializeP0(gcomp, importState, exportState, clock, rc) at all, except you would need to move the logic that is getting profile_memory, cplprint_flag, and dbug elsewhere. (Make sure we actually need those anyway).

And the registration labels in SetServices can be changed:

call NUOPC_CompSetEntryPoint(gcomp, ESMF_METHOD_INITIALIZE, phaseLabelList=(/"IPDv01p1"/), userRoutine=InitializeAdvertise, rc=rc)

should be:

call NUOPC_CompSpecialize(model, specLabel=label_Advertise,  specRoutine=InitializeAdvertise, rc=rc)

And

call NUOPC_CompSetEntryPoint(gcomp, ESMF_METHOD_INITIALIZE, phaseLabelList=(/"IPDv01p3"/), userRoutine=InitializeRealize, rc=rc)

should be:

call NUOPC_CompSpecialize(model, specLabel=label_RealizeProvided,  specRoutine=InitializeRealize, rc=rc)

This also means that the signature of InitializeAdvertise and IntializeRealize can be simplified from:

subroutine InitializeAdvertise(gcomp, importState, exportState, clock, rc)

to

subroutine InitializeAdvertise(model, rc)    
    type(ESMF_GridComp)  :: model    
    integer, intent(out) :: rc

It is not critical that you do this now or in this PR, but it is a good cleanup and removes the confusing IPDxxxx stuff.

Another part of this - which should probably be a follow on, is to rework InitializeAdvertise because it is doing a ton of extra stuff that is not at all related to Advertise. This is all the fcstgridcomp and io component logic. I think it is a bigger project to take this on, and it would be related to a rework of the IO.

@rsdunlapiv
Copy link

The full list of NUOPC semantic labels is here:
http://earthsystemmodeling.org/docs/nightly/develop/NUOPC_refdoc/node4.html#NUOPC_Model

endif
endif

fldname = 't2m'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know JEDI-FV3 interface is using these coupling field names shared between the two components, but it may cause confusion in the future. I'd suggest to use the names in fd_nems.yaml which are shared by all the ufs subcomponents, I think 't2m" has a standard name "inst_temp_height2m". This can be fixed in a later PR.

endif
else if (datatype == ESMF_TYPEKIND_R4) then
if (dimCount == 2) then
call ESMF_FieldGet(exportFields(n),farrayPtr=datar42d,localDE=0, rc=localrc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see only datar83d is used later when setting up the pointers, I am not sure if this will work for 32BIT=Y, for which we may need to set up datar43d. It may not be an issue for fv3-mom6-cice6-wav coupling as those coupled fields are from physics which is always running with real8. We can update this later.

@DusanJovic-NOAA DusanJovic-NOAA merged commit dc5476d into NOAA-EMC:develop May 21, 2021
@DusanJovic-NOAA DusanJovic-NOAA deleted the fv3_exp_imp branch May 21, 2021 19:13
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 23, 2021
with chemistry and other coupled components that
don't require cplflx set to .true.. Fortran
preprocessor macros are introduced to minimize
code replication.
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 23, 2021
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 23, 2021
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 27, 2021
Fortran preprocessor macros with generic subrotines.
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 27, 2021
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 27, 2021
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request May 27, 2021
rmontuoro added a commit to rmontuoro/fv3atm that referenced this pull request Jun 3, 2021
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.

4 participants