-
Notifications
You must be signed in to change notification settings - Fork 13
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
Aquaplanet external gravity wave #187
base: main
Are you sure you want to change the base?
Aquaplanet external gravity wave #187
Conversation
… point the forward step to the correct graph.info file
and a few other small edits
@jeremy-lilly thanks for your work on this, it looks beautiful! Could you add a documentation page to the user's guide, similar to this? |
Trying it out:
This worked similarly on chicoma, perlmutter, and chrysalis. I am using a compiled executable from E3SM-Project/E3SM#6224. Output is as follows:
with plots identical to those above. Beautiful! |
setting up the task (see {ref}`conda-env`). To run a task that has already | ||
been run, it is necessary to first delete `polaris_step_complete.log` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks for adding this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy-lilly, this looks great! The main thing I noticed is that documentation is missing. Please ping me to review again once that gets added.
In the meantime, a few other small comments.
@@ -4,7 +4,7 @@ | |||
convergence_eval_time = 24.0 | |||
|
|||
# Convergence threshold below which a test fails | |||
convergence_thresh = 1.0 | |||
convergence_thresh = 0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, no tests are using this default but I would be curious why it was changed here.
dt_btr_scaling = section.getfloat('dt_btr_scaling') | ||
dt_btr = self.dt / dt_btr_scaling | ||
btr_dt_str = get_time_interval_string(seconds=dt_btr * | ||
self.resolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can make this change without also updating any test cases that already use the split time stepping to use this approach. Maybe none are so far in which case maybe this will be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work here! I made a few comments and suggestions. Don't feel like you have to do all the things I suggested if you don't have the time. Also, I made most of the comments before Xylar posted his, so pardon any conflicting messaging there!
I ran the non-LTS case successfully with E3SM master.
Many of the suggestions have to do with making the convergence framework changes a bit more intuitive since we originally wrote the code for space and time convergence together. It will be great to have the option to evaluate time convergence separately with your changes.
else: | ||
for resolution in self.resolutions: | ||
for dt in dts: | ||
mesh_name = resolution_to_subdir(resolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mesh_name = resolution_to_subdir(resolution) |
Seems this isn't used
mesh_name = resolution_to_subdir(resolution) | ||
forward = dependencies['forward'][dt] | ||
self.add_input_file( | ||
filename=f'{int(dt)}_output.nc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename=f'{int(dt)}_output.nc', | |
filename=f'{int(dt)}s_output.nc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better yet, in case there is a case with fractional seconds.
filename=f'{int(dt)}_output.nc', | |
filename=f'dt{get_time_interval_string(seconds=dt}s_output.nc', |
error.append(error_res) | ||
else: | ||
for i in range(1, len(dts)): | ||
mesh_name = resolution_to_subdir(resolutions[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this implying that when dts
is specified, we are evaluating convergence in time only (and all steps must use the mesh at resolutions[0]
)? If so, I think we should put in some safeguards so that this function cannot be used incorrectly.
@@ -305,7 +382,7 @@ def compute_error(self, mesh_name, variable_name, zidx=None, | |||
|
|||
return error | |||
|
|||
def exact_solution(self, mesh_name, field_name, time, zidx=None): | |||
def exact_solution(self, mesh_name, field_name, time, zidx=None, dt=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it is somewhat of a niche case that the exact solution is just the solution at a fine time step. I would recommend overriding exact_solution
just for your test case. Let me know if you need help finding an example in the code.
@@ -73,13 +86,17 @@ def __init__(self, component, name, subdir, resolution, mesh, init, | |||
self.add_input_file( | |||
filename='init.nc', | |||
work_dir_target=f'{init.path}/initial_state.nc') | |||
if graph_path is None: | |||
graph_path = mesh.path | |||
self.add_input_file( | |||
filename='graph.info', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename='graph.info', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that this is no longer used
config_dt: {{ dt }} | ||
config_time_integrator: {{ time_integrator }} | ||
debug: | ||
config_disable_thick_all_tend: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't include the default options here unless we think it will lead to confusion or having a namelist option set a certain way is essential to the test. You should be able to just specify which terms are turned off (*disable* = true
)
@@ -0,0 +1,81 @@ | |||
omega: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be nice to have one file forward.yaml
that has the config options common to both global and local time stepping and then either/both of forward_global_time_step.yaml
and forward_local_time_step.yaml
that contain the options that are specific to each.
|
||
init_vertical_coord(config, ds) | ||
|
||
temperature_array = temperature * xr.ones_like(ds_mesh.latCell) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temperature_array = temperature * xr.ones_like(ds_mesh.latCell) | |
temperature_array = temperature * xr.ones_like(latCell) |
center_pt = Point(lat_center, lon_center) | ||
for icell in range(0, n_cells): | ||
cell_pt = Point(lat_cell[icell], lon_cell[icell]) | ||
if distance(cell_pt, center_pt) < np.pi / 4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the radius a function parameter
use_progress_bar=use_progress_bar) | ||
|
||
|
||
def label_mesh(mesh, graph_info, num_interface, # noqa: C901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the function to label interface cells different from the one that sets the main regions (1,2). Since we already have an LTS test in compass that may eventually make its way to polaris, it would be nice to create some code here that could be moved to a shared directory down the line and be reused by other LTS tests.
@xylar, @cbegeman, @mark-petersen -- thank you all for your comments and suggestions! Just wanted to let you know that I saw these and that they are on my to-do list, but I might not get to them right away. I will ping everyone when ready for a second look : ) Thanks! |
7ce4884
to
03c72f0
Compare
This PR creates a new ocean task for testing the temporal convergence rate of time-stepping schemes in MPAS-O. The case is that of a simple external gravity wave on an aquaplant. The normal velocity is initialized to zero and the layer thickness is initialized as a Gaussian bump.
The user selects a time-stepping scheme and a series of time-steps to run the test. The first listed time-step will be used to produce a reference solution that other runs will be compared to.
The PR also implements a version of the task for LTS schemes, in particular for the new FB-LTS scheme from this PR: E3SM-Project/E3SM#6224. Convergence plots produced by this task for FB-LTS are given below.
Note that the external gravity wave case was chosen so as to have as few terms in the tendencies as possible -- because of this, both the LTS and FB-LTS schemes show their "true" order of convergence, rather than the first order convergence caused by the operator splitting that both the LTS and FB-LTS codes implement.
Checklist
api.md
) has any new or modified class, method and/or functions listedTesting
comment in the PR documents testing used to verify the changes