-
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
Add convergence tasks for space and time only #236
base: main
Are you sure you want to change the base?
Conversation
TestingI have tested all of the convergence tests on chrys with intel, impi. |
9d40944
to
8c4d8f5
Compare
@sbrus89 If you want to review, feel free! |
@cbegeman, sorry for dropping the ball on this one. I don't seem to get notified when I get marked as a reviewer so I just hadn't noticed. |
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 really like this approach! I also really appreciate the amount of effort that went into it.
I'm seeing a crash in icos/geostrophic/convergence_time/forward/60km_960s
. I think this is too big a time steps to be safe for RK4 at this resolution.
This leads to a more general comment. I think different options like refinement_factor
are appropriate for space
and time
. First, there is not a clear reason that the number of forward steps in time
necessarily needs to match the number in space (though it's fine if it does). But more importantly, one should presumably always be reducing dt
from the base value, whereas it is common to increase the resolution from the base value. So for geostrphic
, I could imagine:
icos_base_resolution = 60.0
icos_space_refinement_factors = 8., 4., 2., 1.
icos_time_refinement_factors = 1., 0.5, 0.25, 0.125
rk4_dt_per_km = 2.0
We might decide that we want to have a cheaper base resolution, we might feel that we could get away with a larger rk4_dt_per_km
, or we might feel we could get away with larger values for icos_time_refinement_factors
but it seems clear that 8 is too large.
A small side note for the future. In a future PR, we might want to add different convergence_thresh
values for space, time and both. We expect 4th order convergence in time only (eventually) with RK4, right? We can cross that bridge when we come to it, though.
for include_viz in [False, True]: | ||
component.add_task(CosineBell(component=component, | ||
config=config, | ||
icosahedral=icosahedral, | ||
include_viz=include_viz)) | ||
for refinement in ['space', 'time', 'both']: |
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.
@cbegeman, what do you think of switching the order of the for loops here? It would make more sense to me to have tests without and with viz next to each other in the list.
Hmm, I'm not happy with my suggested |
I suppose another approach would be to automatically renormalize |
I did intend that eventually we would set time refinement factors differently from space refinement factors with different config options, but it would require a lot of testing for me to figure out what those should be for each case so I thought that could be a follow-on PR. Is that ok with you? |
8c4d8f5
to
62eaaff
Compare
@xylar Ready for re-review when you have a chance |
This PR extends the convergence test framework to accommodate convergence in space, time or both. This PR also adds additional tests for space-only and time-only convergence for the
cosine_bell
,geostrophic
,manufactured_solution
, andinertial_gravity_wave
groups. Theconvergence_time
cases have not been optimized (in terms of resolution and time steps) nor have separate convergence thresholds been specified.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