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

Add shared ocean framework for spherical convergence tests #119

Merged
merged 4 commits into from
Sep 27, 2023

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Sep 21, 2023

This merge adds some shared framework for spherical convergence tests with constant resolution meshes like cosine_bell and soon geostrophic and the various sphere_transport tests.

A new SphericalConvergenceForward parent class does most of the work involved in forward runs for convergence tests, including determining the approximate number of cells in the mesh (for constraining required resources), computing a time step based on the mesh resolution, and setting the run duration and output interval based on a config option.

A set of common (standardized) config options in the spherical_convergence section helps support this approach.

The cosine bell tests have been updated to use the shared framework, and the documentation as also been updated.

We no longer automatically add a task's config options to the config object -- this has to be done in configure(). Cosine bell was the only test so far that relied on this functionality and it did not work quite as expected. It is better to do it manually and control the order in which config options are added from different sources.

Checklist

  • User's Guide has been updated
  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes

@xylar
Copy link
Collaborator Author

xylar commented Sep 21, 2023

Testing

All 4 cosine bell tests have been run successfully on Chrysalis with Intel and OpenMPI with this branch.

@xylar xylar force-pushed the add-spherical-convergence-framework branch from 53bfc6e to f815e98 Compare September 21, 2023 20:31
@xylar xylar marked this pull request as draft September 21, 2023 20:31
@xylar xylar added the in progress This PR is not ready for review or merging label Sep 21, 2023
@xylar xylar force-pushed the add-spherical-convergence-framework branch from f815e98 to 63d09c9 Compare September 22, 2023 07:12
@xylar
Copy link
Collaborator Author

xylar commented Sep 22, 2023

I'm keeping this in draft mode while I work on the geostrophic test because changes may be needed here to accommodate that test.

@xylar xylar force-pushed the add-spherical-convergence-framework branch from 63d09c9 to 34fb2e7 Compare September 22, 2023 07:37
@xylar xylar added enhancement New feature or request ocean Related to ocean tests or analysis and removed in progress This PR is not ready for review or merging labels Sep 23, 2023
@xylar xylar requested a review from cbegeman September 23, 2023 14:39
@xylar xylar marked this pull request as ready for review September 23, 2023 14:39
@xylar xylar self-assigned this Sep 24, 2023
@cbegeman
Copy link
Collaborator

@xylar Since you already have a bit of clean-up in this PR, can you also change "visualizing" to "analyzing" in this line?

A step for visualizing the output from the cosine bell test case

@xylar
Copy link
Collaborator Author

xylar commented Sep 25, 2023

@xylar Since you already have a bit of clean-up in this PR, can you also change "visualizing" to "analyzing" in this line?

A step for visualizing the output from the cosine bell test case

Sure, I can take care of that.

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar I've gotten far enough along in my use of this capability in the sphere_transport tests to be reasonable confident that we don't need any other changes urgently. Once you add the namelist replacement capability and config option for the time to evaluate convergence as I mentioned, I think this is good to go.

@xylar xylar force-pushed the add-spherical-convergence-framework branch 3 times, most recently from df31c30 to b78b27b Compare September 27, 2023 08:32
@xylar
Copy link
Collaborator Author

xylar commented Sep 27, 2023

@cbegeman, sorry of the delay. I was considering trying to fix config options for shared steps before this goes in, but that doesn't seem like a good idea at this point. The changes may be fairly large and this PR is needed for both geostrophic and sphere transport.

Please take a look and make sure everything has been updated as you had requested.

@xylar xylar requested a review from cbegeman September 27, 2023 09:09
@xylar
Copy link
Collaborator Author

xylar commented Sep 27, 2023

All 4 cosine bell test cases are still passing after these changes.

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar Thanks for making those changes! Go for it.

The developer can lose control of whether config options take
precedence over others because the file get added before the
`configure()` method is called.
@xylar xylar force-pushed the add-spherical-convergence-framework branch from d2b00f6 to f639d42 Compare September 27, 2023 17:06
@xylar
Copy link
Collaborator Author

xylar commented Sep 27, 2023

Hmm, some cosine bell changes snuck into #123 on a rebase. They were intended for this branch but will do no harm coming from that one instead, it's just a little confusing.

@xylar
Copy link
Collaborator Author

xylar commented Sep 27, 2023

Thanks very much for the review and discussion, @cbegeman!

@xylar xylar merged commit f532191 into E3SM-Project:main Sep 27, 2023
5 checks passed
@xylar xylar deleted the add-spherical-convergence-framework branch September 27, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants