E3SM-Project / polaris

Testing and analysis for OMEGA, MPAS-Ocean, MALI and MPAS-Seaice
BSD 3-Clause "New" or "Revised" License
6 stars 13 forks source link

Shared ocean spherical convergence analysis step #126

Closed cbegeman closed 1 year ago

cbegeman commented 1 year ago

This PR introduces a new step class, SphericalConvergenceAnalysis that performs the analysis step for a convergence study, computing the convergence rate and generating a figure. Two error measures are available, L2 and L-infinity. The exact_solution method is set to the initial state, but can easily be modified for test cases by overriding this method in their analysis steps.

I also implemented this step in the cosine_bell case to demonstrate its functionality. cosine_bell is BFB with main, but the convergence rates differ because an area-weighted version of the L2 norm is used instead of the original unweighted L2 norm. I removed the differentiation of convergence rates between QU and icosahedral meshes. In my view, the acceptable convergence rates should be set by the solution methods. This change could be reverted by dropping a commit.

I will update the documentation once I get an initial review of the functionality.

Checklist

cbegeman commented 1 year ago

Testing

The cosine bell test cases with viz have been run on Chrys with intel, openmpi.

xylar commented 1 year ago

@cbegeman, I just merged #125. If you can rebase this, that would make the review a little easier. Thank you!

xylar commented 1 year ago

I removed the differentiation of convergence rates between QU and icosahedral meshes. In my view, the acceptable convergence rates should be set by the solution methods. This change could be reverted by dropping a commit.

Okay, that makes sense. In which case the min threshold is set by the QU meshes for all practical purposes. Does it make sense to keep the max? I'm not sure anyone will notice the warning if it gets exceeded.

cbegeman commented 1 year ago

I removed the differentiation of convergence rates between QU and icosahedral meshes. In my view, the acceptable convergence rates should be set by the solution methods. This change could be reverted by dropping a commit.

Okay, that makes sense. In which case the min threshold is set by the QU meshes for all practical purposes. Does it make sense to keep the max? I'm not sure anyone will notice the warning if it gets exceeded.

@xylar I don't see a strong reason to keep the max convergence rate. Are you ok with me removing it from the shared analysis step as well as cosine_bell?

xylar commented 1 year ago

Are you ok with me removing it from the shared analysis step as well as cosine_bell?

Yep, go for it!

cbegeman commented 1 year ago

@xylar Thanks for reviewing! I think I've made all the changes you requested. I also finished editing the documentation. Let me know if you spot anything else.

xylar commented 1 year ago

Okay, @cbegeman, I'm done reviewing for now. If you're okay with some suggested changes, I'll approve and we can get this in! If you want to iterate a bit more, that's fine, too.

cbegeman commented 1 year ago

@xylar I made just about all the changes you suggested. Let me know what you think about the remaining comments.

xylar commented 1 year ago

Looks good! We'll take care of pulling out the one function into polaris.time or something similar in a separate PR.

xylar commented 1 year ago

I reran all 4 cosine bell tests successfully. Aside from the weird change in font size I noted above for future follow-up, everything is great!