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

Fix MPAS-A dycore-only build in version 8.2.0 #1221

Merged

Conversation

kuanchihwang
Copy link
Contributor

@kuanchihwang kuanchihwang commented Aug 2, 2024

When building MPAS-A as a dycore, all physics-related components are disabled. An example scenario of this is for use with CAM/CAM-SIMA.

However, the MPAS-A registry file in version 8.2.0 contains the following regressions, which cause the dycore-only build to break:

  • The MPAS-A registry file introduced a new stream, da_state, which unconditionally includes variables that exist only when physics are enabled.
  • The MPAS-A registry file unconditionally includes a new registry file from Noah-MP, which is applicable only when physics are enabled.

This PR fixes MPAS-A dycore-only build by guarding the above regions with the DO_PHYSICS macro.

For stand-alone MPAS-A, this PR is an NFC (No Functional Change).

When building MPAS-A as a dycore, all physics-related components are disabled.
An example scenario of this is for use with CAM/CAM-SIMA.

However, the MPAS-A registry file in version 8.2.0 contains the following
regressions, which cause the dycore-only build to break:

* The MPAS-A registry file introduced a new stream, `da_state`, which
unconditionally includes variables that exist only when physics are enabled.
* The MPAS-A registry file unconditionally includes a new registry file from
Noah-MP, which is applicable only when physics are enabled.

Fix MPAS-A dycore-only build by guarding the above regions with the
`DO_PHYSICS` macro.
@mgduda mgduda self-requested a review August 2, 2024 20:27
@mgduda mgduda requested a review from gdicker1 August 6, 2024 15:55

# If MPAS_CAM_DYCORE is found in CPPFLAGS, PHYSICS will become undefined automatically
#
ifeq ($(findstring MPAS_CAM_DYCORE,$(CPPFLAGS)),)
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I agree that this is a reasonable change in principle, I think it may be outside the scope what we want to include in the v8.2.1 release. My understanding is that CAM's build already ensured that DO_PHYSICS was not defined, and the need for this change may be due to the reuse of makefiles in ongoing work on CAM-SIMA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, CAM maintains its own Makefiles so this change is not needed. This change is for CAM-SIMA because it tries to be as close to the upstream MPAS-A as possible.

I have dropped the commit b3fc19a. The PR descriptions were also updated to no longer mention physics being disabled for dycore-only build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fully support creating a separate PR to the develop branch with this MPAS_CAM_DYCORE logic.

@kuanchihwang kuanchihwang force-pushed the stg/fix-dycore-only-build branch from b3fc19a to 520d165 Compare August 6, 2024 16:55
@gdicker1
Copy link
Collaborator

gdicker1 commented Aug 6, 2024

I've confirmed that CAM using MPAS v8.2.01 fail to build and give the message

ERROR: Dimension on variable acc_etrani in var_struct diag_physics_noahmp not defined.

After merging these changes, builds can now succeed!

Footnotes

  1. requires some code updates that I'll make a PR for in ESCOMP/CAM

Copy link
Collaborator

@gdicker1 gdicker1 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 to me and builds succeed!

@mgduda mgduda self-requested a review August 6, 2024 18:05
Copy link
Contributor

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

I've verified that the changes in this PR fix dynamics-only builds of stand-alone MPAS-A.

@mgduda mgduda merged commit 93e2ac5 into MPAS-Dev:hotfix-v8.2.1 Aug 6, 2024
@kuanchihwang kuanchihwang deleted the stg/fix-dycore-only-build branch August 6, 2024 20:10
mgduda added a commit that referenced this pull request Aug 7, 2024
This merge addresses several issues in the MPAS-Atmosphere model and in the MPAS
infrastructure. Specific changes in this merge include:

* Improved detection of an 'mpi_f08' module (PR #1202), as well as improved
  detection of netCDF and PnetCDF library paths (PR #1203), in the top-level
  Makefile.

* The addition of a missing dependency in the physics Makefile to correct
  parallel build issues (PR #1204).

* Fixes to the CMake build files used by MPAS-JEDI (PR #1205).

* Fixes to double-precision builds of MPAS-Atmosphere (PR #1207, PR #1208).

* Correction of the calculation of height AGL used in the computation of 1-km
  radar reflectivity fields (PR #1213).

* Correction of an issue that prevented the MYNN PBL scheme from being used
  without also using the Thompson aerosol-aware microphysics (PR #1215).

* Fixes to allow MPAS-Atmosphere to be built without physics (i.e., dynamics-
  only builds) (PR #1221).

* Various code cleanup and minor corrections (PR #1206, PR #1212, PR #1224,
  PR #1226).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants