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

Clean up checks for convergence and limiters #361

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Conversation

dennisYatunin
Copy link
Member

@dennisYatunin dennisYatunin commented Feb 1, 2025

Purpose

This PR cleans up the code we use to check convergence and limiters in our CI, and it enforces strict tests for algorithm convergence.

There are two significant issues that are not addressed in this PR:

  • The current setup recomputes the numerical reference solutions for every algorithm's ClimaCore convergence tests. This is somewhat acceptable for our current tests, but it will be prohibitively expensive once we add a 3D test. Instead, we should compute each reference solution once and reuse it for every algorithm. A large chunk (possibly most) of our CI time is already being spent on recomputing reference solutions, and this problem will only be exacerbated as we add better tests.
  • The limiter table shows that SSP333 is currently not acting as an SSP method when used with a limiter, since its overshoots and undershoots are nearly identical to those of ARS343. This means that I cannot add a strict test for limiters like I did for convergence. When the table was first implemented, the overshoots and undershoots of SSP333 were close to machine epsilon, so a bug must have been introduced at some point.

Overall, I think our ERK, ARK, and Rosenbrock algorithms are functioning correctly in the absence of limiters. Hopefully the tests I've added will catch convergence issues in the future, such as the one that was introduced in v0.7.39.

Content

  • Remove duplicated and unnecessary code from compute_convergence.jl and summarize_convergence.jl
  • Remove plotting_utils.jl, since it is no longer necessary
  • Remove non-numerical values from the JLD2 files, and ensure that the remaining values are sufficient to run summarize_convergence
  • Reduce the number of JLD2 files generated by each convergence test job in CI from 6 to 1, and move them to the output folder
  • Add missing convergence test jobs for ARK437L2SA1 and ARK548L2SA2
  • Remove unnecessary artifacts from the summary jobs in CI
  • Remove duplicate entries from the limiter summary table
  • Modify variable and function names to clarify what the convergence and limiter analyses are doing
  • Remove missing implicit tendencies from the 2D explicit heat equation problem so that it can be used to test convergence
  • Modify benchmark_step so that it can handle missing implicit/explicit tendencies
  • Modify summarize_convergence so that it can plot the results for algorithms that failed to converge
  • Add strict convergence tests for all algorithms and all problems (except for the 1D implicit heat equation problem, which has several broken tests because it appears to be outside the stability regions of some IMKG algorithms)
  • Add a strict test for consistency of unconstrained and SSP algorithms

  • I have read and checked the items on the review checklist.

@dennisYatunin dennisYatunin changed the title Clean up convergence and limiter tests Clean up checks for convergence and limiters Feb 1, 2025
@dennisYatunin dennisYatunin force-pushed the dy/plot_cleanup branch 2 times, most recently from f2217cf to 95f5e3c Compare February 2, 2025 04:02
@dennisYatunin dennisYatunin linked an issue Feb 2, 2025 that may be closed by this pull request
@dennisYatunin dennisYatunin force-pushed the dy/plot_cleanup branch 8 times, most recently from b7e2e13 to b25a9e7 Compare February 3, 2025 22:21
Copy link
Member

@charleskawczynski charleskawczynski 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, thanks for cleaning this up!

@dennisYatunin dennisYatunin merged commit 2bd65cc into main Feb 7, 2025
8 of 9 checks passed
@dennisYatunin dennisYatunin deleted the dy/plot_cleanup branch February 7, 2025 00:38
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.

Add tests to convergence plots
2 participants