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

Make xgrid the default aoflux_grid when atm and ocn are running on different grids #515

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

billsacks
Copy link
Member

Description of changes

Use the exchange grid for the atmosphere-ocean flux calculation unless atm & ocn are on the same grid.

Also adds a test of aoflux_grid=ogrid to the aux_cmeps test suite so that that configuration continues to be tested.

Specific notes

Contributors other than yourself, if any: Consultation with @mvertens on how we should set the default – specifically, continuing to use ogrid when atm & ocn are on the same grid.

CMEPS Issues Fixed (include github issue #):

Are changes expected to change answers? YES - significant answer changes for any configuration where atmosphere-ocean flux calculations are performed and atm & ocn are on different grids.

Any User Interface Changes (namelist or namelist defaults changes)? Changes default value for aoflux_grid

Testing performed

Please describe the tests along with the target model and machine(s)
If possible, please also added hashes that were used in the testing

Ran the full CESM prealpha and prebeta test suites on derecho using cesm3_0_beta03, but with the following commits cherry-picked into CMEPS:

I compared my failures with failures in the baselines from @fischer-ncar 's testing. I got a number of additional failures due to tests exceeding walltime limits, which I'm ignoring (7 failures in prealpha and 7 failures in prebeta). I also got a failure in PET_PM.ne30pg3_t232.BLT1850.derecho_intel.allactive-defaultiomi and the similar gnu test, PET_PM.ne30pg3_t232.BLT1850.derecho_gnu.allactive-defaultiomi, with an error that seems unrelated to the xgrid: "CAM configure: ERROR: The SE dycore does not currently work with threading on." Other than those, tests passed.

@billsacks billsacks requested a review from jedwards4b October 21, 2024 22:18
@billsacks
Copy link
Member Author

@jedwards4b - based on discussion with Bob Oehmke, it might still be at least a couple more weeks before his exchange grid fixes make it into ESMF. I am therefore thinking that we should bring in the change to using xgrid by default (i.e., this PR) soon rather than waiting for the updated ESMF version. Does that make sense to you?

However, this should come in after #514 is merged.

Comment on lines +68 to +70
<test compset="X" grid="f19_g17" name="ERS_D_Ln9" testmods="drv/aoflux_ogrid">
<machines>
<machine name="derecho" compiler="intel" category="aux_cmeps"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

I felt like it would be good to have at least one test that still covers doing the aoflux calculation on the ogrid to ensure that that configuration continues to work. It felt right to add this to aux_cmeps since this is about testing cmeps code, but I'm not sure how often aux_cmeps is actually run. @jedwards4b do you have feelings on whether aux_cmeps is reasonable or if we should add this to prealpha or prebeta (instead or in addition)?

Also, I wasn't positive if an X compset actually covers the atm-ocn flux calculation, and so if this X compset test is sufficient for exercising aoflux_grid=ogrid. @jedwards4b or @mvertens do you know? If you aren't sure, I can try to confirm that we're covering relevant code with this test, or just change it to a B compset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that retaining at least one ogrid case is a good idea.

@billsacks
Copy link
Member Author

this should come in after #514 is merged

That PR has been merged. So I feel like this can come in at any point. Outstanding questions are:

  • Are we ready to bring in this change to xgrid being the default? (@jedwards4b )
  • Is the added test here a reasonable one for continuing to test ogrid, both in terms of the test suite (aux_cmeps) and compset (X)? (@jedwards4b / @mvertens )

Copy link
Collaborator

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

thank you!

Comment on lines +68 to +70
<test compset="X" grid="f19_g17" name="ERS_D_Ln9" testmods="drv/aoflux_ogrid">
<machines>
<machine name="derecho" compiler="intel" category="aux_cmeps"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that retaining at least one ogrid case is a good idea.

@jedwards4b
Copy link
Collaborator

I think maybe if you rebase it will fix the workflow error.

@billsacks
Copy link
Member Author

@jedwards4b - updating the PR to latest main didn't solve the workflow issue ("ModuleNotFoundError: No module named 'distutils'")

@jedwards4b
Copy link
Collaborator

THis is fixed in the latest cime, updating to the latest cmeps should get the latest cime, anyway I understand what the issue is and it isn't your PR.

@jedwards4b jedwards4b merged commit 27f4ecb into ESCOMP:main Oct 24, 2024
1 of 2 checks passed
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.

Change CESM's default aoflux_grid to xgrid
2 participants