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

Bug fixes for RRFS 32-bit physics support, and minor feature additions #1797

Merged
merged 84 commits into from
Jul 10, 2023

Conversation

SamuelTrahanNOAA
Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA commented Jun 15, 2023

Description

Fixes two bugs and adds features:

  1. aerinterp.F90 in CCPP stepped all over memory when kind_phys=4 because it read 8-byte reals into that array.
  2. aerinterp.F90 did not report NetCDF errors up the call tree via errmsg & errflg
  3. add regression tests for rrfs conus13km 32-bit physics and -DFASTER=ON
  4. Initialize some uninitialized variables in GFS_typedefs.F90
  5. Changes the COMPILE lines to use names instead of numbers so we don't have to renumber the entire rt.conf when adding a compilation line

Input data additions/changes

  • No changes are expected to input data.

Anticipated changes to regression tests:

  • Changes are expected to the following tests
    New regression tests for 32-bit physics in RRFS

Subcomponents involved:

  • AQM
  • CDEPS
  • CICE
  • CMEPS
  • CMakeModules
  • FV3
  • GOCART
  • HYCOM
  • MOM6
  • NOAHMP
  • WW3
  • stochastic_physics
  • none

Combined with PR's (If Applicable):

Commit Queue Checklist:

  • Link PR's from all sub-components involved in section below
  • Confirm reviews completed in ALL sub-component PR's
  • Add all appropriate labels to this PR.
  • Run full RT suite on either Hera/Cheyenne AND attach log to a PR comment.
  • Add list of any failed regression tests to "Anticipated changes to regression tests" section.

Linked PR's and Issues:

Testing Day Checklist:

  • This PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR.
  • Move new/updated input data on RDHPCS Hera and propagate input data changes to all supported systems.

Testing Log (for CM's):

  • RDHPCS
    • Hera
    • Orion
    • Jet
    • Gaea
    • Cheyenne
  • WCOSS2
    • Dogwood/Cactus (IKE Day, skipping)
    • Acorn
  • CI
    • Completed
  • opnReqTest
    • N/A
    • Log attached to comment

@DeniseWorthen
Copy link
Collaborator

@BrianCurtis-NOAA I had the same issue, and found hashing out lines 135 through 138 in rt.sh worked. However this is probably not the preferred fix. Otherwise I think you can export ACCNR prior to running.

Are you editing code during testing to get it working?

If you are, it needs to be brought back into the PR, discussed, or not changed at all.

We need to understand---are the RTs being tested with uncommitted changes?

@jkbk2004
Copy link
Collaborator

I agree with forcing along with line 216 in getopts block.

@zach1221
Copy link
Collaborator

@DeniseWorthen No, sorry. To clarify you can use the export command, for example "export ACCNR=${ACCNR:-epic}", on Orion, before running ./rt.sh. It doesn't require any changes to the rt.sh script. This is how I ran the tests, but I thought I'd offer the other workaround as well, while acknowledging it's likely not preferred.

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Jul 10, 2023

@zach1221 @jkbk2004 Have any RT logs been posted for cases where the PR was run w/o committed changes?

Edit: Thanks for the clarification.

@SamuelTrahanNOAA
Copy link
Collaborator Author

Do people want me to make changes or not? If so, what are the changes? I'm seeing too many conflicting comments.

@jkbk2004
Copy link
Collaborator

Do people want me to make changes or not? If so, what are the changes? I'm seeing too many conflicting comments.

@SamuelTrahanNOAA Please, go ahead to add fix. Sounds like option to force somewhere inside getopts block

@BrianCurtis-NOAA
Copy link
Collaborator

BrianCurtis-NOAA commented Jul 10, 2023

Do people want me to make changes or not? If so, what are the changes? I'm seeing too many conflicting comments.

Move lines: +134 to +141 to 216 (after the while getopts section)

@SamuelTrahanNOAA
Copy link
Collaborator Author

Move lines: +134 to +141 to 216 (after the while getopts section)

I tested that fix, and it works. With -a, the workflow uses the account I give it. Without -a, it aborts with the expected message.

The fix is in my branch now.

@jkbk2004 jkbk2004 mentioned this pull request Jul 10, 2023
35 tasks
@BrianCurtis-NOAA
Copy link
Collaborator

@jkbk2004 @zach1221 other than Cheyenne, give it a look over and I assume you can start the merging.

@zach1221
Copy link
Collaborator

Thanks, @BrianCurtis-NOAA . Yes, since we're skipping Cheyenne, testing is complete and we can begin the merging process.

@dustinswales
Copy link
Collaborator

@zach1221 Physics has been merged.
@SamuelTrahanNOAA You can go ahead and update your FV3 submodule pointer.

@SamuelTrahanNOAA
Copy link
Collaborator Author

You can go ahead and update your FV3 submodule pointer.

I updated the fv3atm's submodule pointer to ccpp-physics. It now points to the authoritative ufs/dev branch. You can merge the fv3atm PR.

@zach1221
Copy link
Collaborator

FV3 sub-pr is merged. Please update the submodule pointer and revert the .gitmodules for FV3. @SamuelTrahanNOAA

@SamuelTrahanNOAA
Copy link
Collaborator Author

FV3 sub-pr is merged. Please update the submodule pointer and revert the .gitmodules for FV3

The deed is done. We are ready for final approvals and a merge.

@jkbk2004 jkbk2004 self-requested a review July 10, 2023 19:47
@zach1221 zach1221 merged commit f1f0180 into ufs-community:develop Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jenkins-ci Jenkins CI: ORT build/test on docker container Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add compilation lines without updating every COMPILE in the rt.conf
8 participants