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

Enable GOCART integration in UFS #310

Merged

Conversation

rmontuoro
Copy link
Collaborator

@rmontuoro rmontuoro commented May 24, 2021

Description

This PR adds the required infrastructure to couple with UFS-Aerosols, a prognostic aerosol model based on NASA GOCART (see #309 and UFS weather model issue #590).

It also includes a hotfix to prevent segfault errors when coupling with chemistry (see PR #308)

Features:

  • Implemented sharing of atmospheric tracer metadata (name, units) to coupled model component via a standard NUOPC connector if coupling tracer field is accessed by shared memory reference
  • Introduced additional metadata to tracer records in field_table to identify and validate prognostic and diagnostic chemistry tracers. A new record line starting with the tracer_usage keyword is now required to recognize chemistry tracer as follows:
      "tracer_usage", "chemistry"                    (default, prognostic)
      "tracer_usage", "chemistry", "type=diagnostic" (diagnostic)
  • Added sanity check for input chemistry tracers, ensuring that both prognostic and diagnostic tracers in field_table are contiguous, and diagnostic tracers follow prognostic ones.
  • Improved algorithm to set aerosol scavenging factors
  • Exported instantaneous fields for:
    • 3D non-convective liquid and ice precipitation fluxes
    • 3D cloud fraction
    • normalized soil wetness
    • ice fraction (as used in the atmospheric model)
    • lake fraction
    • ocean fraction
    • surface snow area fraction
  • Enabled import of diagnostic tracers

Fixes:

  • Prevent a divide by zero error when costant arrays are rescaled for lossy compression in write_netcdf_parallel()
  • Correctly sample and compute elapsed time for field bundle regridding operations.
  • Include hotfix (see PR Fix segfault errors in coupled FV3-GOCART after PR #301 #308) to allow latest setup_exportdata() to work with coupled chemistry.

This PR also eliminates legacy code used for GEFS-Aerosols coupling and no longer needed.

Issue(s) addressed

Testing

Successful regression tests performed for:

  • hera.intel
  • orion.intel
  • wcoss_dell_p3

Log files available at: https://github.com/rmontuoro/ufs-weather-model/tree/feature/gocart

This PR does not change the regression test baseline

Dependencies

large scale rain, and convective rain at the end of
each coupling step if coupling with chemistry model.
in noah/osu land-surface model subdriver.
band layer cloud optical depths (0.55 and 10 mu channels)
to prevent floating invalid errors due to uninitialized
optical depth arrays.
coupling array at the beginning of each coupling step
if coupled with chemistry model.
the NUOPC Realize phase since it breaks coupling
with aerosol component.
via memory reference by default, if requested.
for timing of field bundle regridding operations.
coupled aerosol model if unavailable.
them to file to prevent dividing by zero.
coupled aerosol component (GEFS-Aerosols, GOCART).
@rmontuoro rmontuoro requested a review from DusanJovic-NOAA June 3, 2021 19:31
@rmontuoro
Copy link
Collaborator Author

The latest updates implement option 2a and enforce pointer association as agreed. @DusanJovic-NOAA, @DeniseWorthen , and @junwang-noaa please let me know if this works for you

atmos_model.F90 Outdated
Comment on lines 2602 to 2603
case default
localrc = ESMF_SUCCESS
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default case should be an error, not success.

Copy link
Collaborator Author

@rmontuoro rmontuoro Jun 3, 2021

Choose a reason for hiding this comment

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

@DusanJovic-NOAA - setup_exportdata() is not comprehensive. For instance, exported fields used by the chemistry model are not included in this subroutine and should not be at this time since they are exported at the end of FV3 run phase 1.
setup_exportdata() is used during FV3 initialization and by update_atmos_model_state(), which is called in run phase 2, except when FV3 is coupled with chemistry.

I understand the motivation, but setting the default case to be an error would break the coupled model.
Again, refactoring this code should not be part of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

setup_exportdata is not used if cplchem is true, so the fact that not all chemistry fields are exported is irrelevant.

Why would it break the coupled model? Are there still exported fields (other than the 'chemistry' fields) that are not handled by the 'select case'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setup_exportdata() is called by InitializeRealize() (fv3_cap.F90) even if cplchm is .true., and this is where all those issues are coming from.

If setup_exportdata() was not intended to be used with the coupled chemistry model, I'd be happy to disable it when cplchm is set to .true.. Please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not know if you need to call setup_exportdata with the chemistry model from InitializeRealize or not. But if you do then add all exported fields, and set the default case to error. If the default case is not an error there's no way to catch typos. For example, if instead:
case ('o3mr')
I write:
case ('o3nr')
model will silently continue to run, without me noticing that o3mr is not exported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setup_exportdata() is not required at all when coupling with the chemistry model. I do agree with you that, generally speaking, we should return an error when a mismatch occurs.

I suggest:

  1. checking if cplchm is .true. inside setup_exportdata() and bailing out if so (this is needed since GFS_control%cplchm is not accessible in the FV3 cap)
  2. returning with an error in the default case, as you suggest.

Would this work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DusanJovic-NOAA - I've implemented the changes above. Please take a look.

exported ESMF field in setup_exportdata().
@rmontuoro rmontuoro requested a review from DusanJovic-NOAA June 3, 2021 20:50
@junwang-noaa
Copy link
Collaborator

I replied in email, but I was told there is a temporary issue with github. I reposted the message here.

Raffaele, I think we discussed this at the last fv3-jedi meeting, we should use one single interface setup_exportdata for all the applications including fv3-mom6/cice6/ww3, fv3-jedi and fv3-chem. If there are fields used by several applications with the same names but associated with different fv3 internal fields, we can use namelist variables (e.g. cplchm) to set up the correct field value. Also the setup_exportdata can be called at InitializeRealize() if the internal coupled fields are properly initialized. It will be called again at the end of fv3 forecast phase1, so you will get updated fv3 export fields before forecast phase2 is called, I don't think there is any issue there. Using both setup_exportdata and update_atmos_chemistry for setting up fv3 export fields are causing confusion. @rsdunlapiv @@theurich @mark-a-potts can comment on this.

@climbfuji climbfuji requested a review from junwang-noaa June 30, 2021 22:25
@climbfuji
Copy link
Collaborator

I updated the ccpp-physics submodule pointer following the merge of NCAR/ccpp-physics#664 and reverted the change to .gitmodules. Please check @DusanJovic-NOAA @junwang-noaa @rmontuoro

Copy link
Collaborator

@DusanJovic-NOAA DusanJovic-NOAA 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.

call block_data_copy_or_fill(datar82d, DYCORE_data(nb)%coupling%z_bot, zeror8, Atm_block, nb, rc=localrc)
!--- JEDI fields
case ('u')
call block_atmos_copy(datar83d, Atm(mygrid)%u, Atm_block, nb, 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.

Do you run fv-gocart test with 64bit? the Atm(mygrid)%u can be real(4) is compiled with 32BIT=Y.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This block is not used when coupling with GOCART.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We may have issue for this with 32BIT=Y, but this can be fixed later.

deallocate(arrayr4_3d_save)
call mpi_allreduce(mpi_in_place,compress_err(i),1,mpi_real4,mpi_max,mpi_comm,ierr)
!print *,'field name=',trim(fldName),dataMin,dataMax,compress_err(i)
if (scale_fact > 0.) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is original code causing issues or the code changes are to skip the allocating array and mpi calls for constant field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is meant to address an issue in the original code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know what is the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a divide by zero if scale_fact = 0

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.

The code changes are working. Please see some questions of mine.

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.

Coupled FV3-GOCART system crashes after PR #301
5 participants