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

Shared ocean spherical convergence analysis step #126

Merged

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Oct 2, 2023

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

  • 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
    New tests have been added to a test suite

@cbegeman cbegeman requested a review from xylar October 2, 2023 21:07
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 2, 2023

Testing

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

@xylar
Copy link
Collaborator

xylar commented Oct 3, 2023

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

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

This is going to be amazing to have! It was an excellent idea!

I have a few changes to suggest. And there's obviously some documentation needed.

polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
Comment on lines +325 to +403
t0 = datetime.datetime.strptime(xtime[0].decode(),
'%Y-%m-%d_%H:%M:%S')
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's great to know that this works! I think I had had trouble with years lower than around 1600 in the past but maybe this has been fixed. I'm also surprised that the xtime variable can be decoded with decode(). This must be a recent improvement in xarray.

Copy link
Collaborator Author

@cbegeman cbegeman Oct 3, 2023

Choose a reason for hiding this comment

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

This reminds me -- I was wondering if there is another common place this should live in the framework. I could see us invoking this in a lot of analysis steps. Or maybe this should live in MPAS-Tools?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could have a polaris.time module and put that there? It would be really handy to have something equivalent for converting from seconds to xtime values, and that could also go there in the future.

polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/tasks/cosine_bell/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/tasks/cosine_bell/analysis.py Outdated Show resolved Hide resolved
@xylar
Copy link
Collaborator

xylar commented Oct 3, 2023

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 cbegeman force-pushed the ocn-shared-convergence-analysis-step branch 2 times, most recently from 9c9f4b1 to 611da50 Compare October 3, 2023 16:31
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 3, 2023

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
Copy link
Collaborator

xylar commented Oct 3, 2023

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

Yep, go for it!

@cbegeman cbegeman force-pushed the ocn-shared-convergence-analysis-step branch from 611da50 to 5550746 Compare October 3, 2023 18:41
@cbegeman cbegeman force-pushed the ocn-shared-convergence-analysis-step branch from 5550746 to 52a5517 Compare October 3, 2023 21:23
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 3, 2023

@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 xylar self-requested a review October 4, 2023 12:35
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

A few more small changes to consider. In the meantime, I'll test this out.

polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/tasks/cosine_bell/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/tasks/cosine_bell/analysis.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

And a few more things that I noticed in running the cosine bell and building the docs.

polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/tasks/cosine_bell/analysis.py Outdated Show resolved Hide resolved
docs/developers_guide/ocean/framework.md Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
polaris/ocean/convergence/spherical/analysis.py Outdated Show resolved Hide resolved
@xylar
Copy link
Collaborator

xylar commented Oct 4, 2023

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.

Co-authored-by: Xylar Asay-Davis <[email protected]>
@cbegeman cbegeman force-pushed the ocn-shared-convergence-analysis-step branch from 60a6fd9 to 0f01b61 Compare October 4, 2023 21:59
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 4, 2023

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

@xylar
Copy link
Collaborator

xylar commented Oct 5, 2023

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

order1 = 0.5 * error_array[-1] * (res_array / res_array[-1])
order2 = 0.5 * error_array[-1] * (res_array / res_array[-1])**2

fig = plt.figure()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to use the matplotlib style file that we use in the global viz plots.

We would need these imports (from polaris.viz.globe)

import sys
from typing import TYPE_CHECKING  # noqa: F401

if TYPE_CHECKING or sys.version_info >= (3, 9, 0):
    import importlib.resources as imp_res  # noqa: F401
else:
    # python <= 3.8
    import importlib_resources as imp_res  # noqa: F401
...

and then:

Suggested change
fig = plt.figure()
style_filename = str(
imp_res.files('polaris.viz') / 'polaris.mplstyle')
plt.style.use(style_filename)
fig = plt.figure()

Without this, the plots have different font sizes before and after the viz has run:
convergence_icos
convergence_qu

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's decide if we want to do this in a separate PR...

@xylar
Copy link
Collaborator

xylar commented Oct 5, 2023

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!

@xylar xylar merged commit 9a696f8 into E3SM-Project:main Oct 5, 2023
5 checks passed
@xylar xylar deleted the ocn-shared-convergence-analysis-step branch October 5, 2023 09:55
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.

2 participants