Skip to content

Init TotalVerticalTransportPseudoVelocity for testing. - #575

Open
overfelt wants to merge 2 commits into
E3SM-Project:developfrom
overfelt:overfelt/InitTotalVerticalTransportPseudoVelocity
Open

overfelt wants to merge 2 commits into
E3SM-Project:developfrom
overfelt:overfelt/InitTotalVerticalTransportPseudoVelocity

Conversation

@overfelt

@overfelt overfelt commented Sep 25, 2026 •

Copy link
Copy Markdown

Since tendencies now use TotalVerticalTransportPseudoVelocity, initialize to the same values as VerticalPseudoVelocity and TotalVerticalPseudoVelocity. Fixes #573

Do a running total of errors instead of zeroing out each time.

Checklist

  • Building

    • CMake build does not produce any new warnings from changes in this PR
  • Testing

    Perlmutter

    • CTests Pass
    • Polaris omega_pr Pass

Since tendencies now use TotalVerticalTransportPseudoVelocity,
initialize to the same values as VerticalPseudoVelocity and
TotalVerticalPseudoVelocity.

Do a running total of errors instead of zeroing out each time.
@overfelt
overfelt requested a review from xylar September 28, 2026 14:18
@xylar

xylar commented Sep 28, 2026

Copy link
Copy Markdown

Thanks @overfelt. Taking a look now.

@xylar

xylar commented Sep 28, 2026

Copy link
Copy Markdown

Testing

This fixes the inf in FCTHNewInv and makes the FCT checks fail CTest, but two TEND tests now fail on Chrysalis. I ran CTests with Intel (Release) on the PR base c1aacdc2d7 and on this branch, with #574 merged into both so they configure (#572), and the 260911 sphere mesh. I also ran this branch with the new TotalVerticalTransportPseudoVelocity init line removed, to check that the inf is now caught:

CTest FCTHNewInv
base 50/50 pass inf on plane, single-precision plane and sphere
this PR 48/50 pass finite
this PR without the init line 47/50 pass inf, all 3 TEND tests fail

The two remaining failures:

  • TEND_SPHERE_TEST: FCTHNewInv LInf FAIL, expected 3.0541724683419424e-05, got 3.0543973828844884e-05. That expected value was never testable while the result was inf; the new value matches ExpectedFCTHInv to 9 digits.
  • TEND_PLANE_SINGLE_PRECISION_TEST: FCTHProv, FCTHProvInv and FCTHNewInv (expected 0, got 1.5e-05), FCTHighAndLowOrderFlux_High and FCTAccumulateHighOrderFlux_0. These are the single-precision failures from FCT tendency tests swallow failures; FCTHNewInv is inf #573, unchanged from the base. All five compare near-zero expected errors with an ATol of 1e-10, which single precision cannot meet.

Work directory: /lcrc/group/e3sm/ac.xylar/polaris_1.1/chrysalis/test_20260928/omega575


Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly.

@xylar xylar left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved based on a look at the code changes and my testing with Claude.

Whoops, I jumped the gun. Need to take a more careful look.

@xylar xylar self-assigned this Sep 28, 2026
@xylar xylar added the bug Something isn't working label Sep 28, 2026
@xylar
xylar self-requested a review September 28, 2026 15:49
@xylar

xylar commented Sep 28, 2026

Copy link
Copy Markdown

@overfelt, I approved too soon. (Multitasking gone awry.)

Would you be able to look at the new failing tests on Chrysalis? I don't know my way around the CTests you added well enough to understand the failures yet.

I can give it a go if you can't.

@xylar xylar left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both remaining failures from my testing comment look like test-constant fixes. I haven't rebuilt with these changes.

For TEND_SPHERE_TEST, the LInf in the spherical ExpectedFCTHNew (line 222) needs to become 3.0543973828844884e-05, the value from my run; the L2 already passes. The old value predates the regenerated sphere mesh: 1a564eb updated ExpectedFCTHProv and ExpectedFCTHInv, but FCTHNewInv was inf at the time.


Posted by Claude Code on @xylar's behalf. The testing, analysis and wording above are AI-authored; please check them accordingly.

const auto HProv = TrHorzAdvOnC.GetHProv();
const auto HNewInv = TrHorzAdvOnC.GetHNewInv();

const Real ATol = 1.0e-10;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The single-precision failures are round-off, up to 1.5e-5 against expected values of 0 or ~1e-16. With RTol scaling those tiny values, this ATol sets the tolerance, and 1e-10 is too tight for floats. Making it precision-dependent like RTol at line 1979 would cover all five on Chrysalis:

const Real ATol = sizeof(Real) == 4 ? 1e-4 : 1e-10;

That is needed here and at lines 1495, 1541 and 1672, or once at the top of the function.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to test in Chrysalis but can't access the platform and assumed it was down for maintenance. Maybe something wrong with my account that I need to fix.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Chrysalis isn't down so indeed it seems like you need to check with @rljacob about getting your account reactivated once he's back.

@mwarusz mwarusz mentioned this pull request Sep 28, 2026
8 of 22 tasks

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FCT tendency tests swallow failures; FCTHNewInv is inf

2 participants