-
Notifications
You must be signed in to change notification settings - Fork 99
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
Updating step size control #483
Conversation
…ssic. Remove step_index reset to sharpclaw solver.
…. step_index is updated only when step is accepted.
Also: - Change LMM names from 'MS' to 'LMM' - Make step rejection work correctly for LMMs - Allow use of grid norms in util.check_diff() by passing delta
Due to step rejection occurring since we now check the CFL number for all RK stages.
…tore them. Also improve the way stored information from previous steps is updated.
…lobal cfl at the beggining of each step.
cb9869f
to
419a068
Compare
Any ideas on the test failure front? It would be nice to merge this in but I really don't think we should if the tests fail even if it is not reproducible. |
No it is unclear why this single test fails. I run nosetests on both my mac and Ubuntu with no errors. |
The tests pass although PETSc does emit an error I have not seen before
I am not sure if that's anything to do with the problem you are seeing as I cannot figure out which test is actually emitting the error and why it does not cause the test to fail. |
@ketch thanks for tracking that one down. |
I can reproduce Travis error message when running
Before I was just running In my opinion this error is not caused by the recent changes in step-size control. It is an error related to TVD discretizations (
The error occurs when testing sharpclaw with TVD methods ( You can test it out by checking Then run This might be related to errors accumulated at ghost cells or might be more involved and has to do with how TVD methods work in MPI and PETSc. |
@@ -22,6 +22,8 @@ def get_cached_max(self): | |||
def set_local_max(self,new_local_max): | |||
self._global_max = new_local_max | |||
|
|||
def update_global_max(self,new_local_max): | |||
self._global_max = new_local_max | |||
def set_global_max(self,new_global_max): |
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 think this method is identical to that of the parent function, so there is no need to explicitly duplicate it here. It will be automatically inherited.
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.
Correct. I removed that method. Also set_local_max
is not currently being used.
@ketch Do you think we should test LMM only with WENO and merge this PR? |
Add set_global back to CFL object. Only update dt_list and dtFE_list when a step is accepted.
…eject_step function.
…eject_step function (2).
Excellent, I wanted to make sure as we had talked about making this change before. I would say we should merge this then unless someone else has objections. |
There is some space for improvements, for example using the already saved function evaluations for a general LMM method and I recently discovered that we are using an additional register for the SSP104 method. But these are smaller changes and can be included in a forthcoming PR. |
Yeah, I would say merge this and add those as upcoming enhancements. There are a few spots that I think could be PEP8ized as well but that's pretty minor. |
Also: - General LMM now uses previously saved function evaluations - rename self.deltaq to self.dq_dt - update accept_reject_step and get_dt_new
I have fixed the issue regarding the step-size choice for starting methods and also make general LMM use previous function evaluations. |
…nto step-size_control * 'step-size_control' of git://github.com/hadjimy/pyclaw: Update nosetests. Correct step-size choice for starting methods of LMMs. Conflicts: src/pyclaw/sharpclaw/solver.py
This avoids some messy things and ensures that the step size rejection is determined using the right CFL number.
Also: - Add SSPLMM53 - Edit documentation
- Correct omega3 in SSPLMM43. - Add SSP coefficients for 3rd order SSPLMM. - Don't pass cfl to accept_reject_step(). - Make accept_step local variable in accept_reject_step(). - Use [:] instead of copy. - Edit documentation/remove redundant comments.
with the desired order.
self.step(). - Make SSPLMM work with constant step size.
- Make a single routine to update saved info for LMMs at the beggining of step() function. - Step size for starting values is chosen inside get_dt_new() and when the linear multistep part is active, the step size is set in set_dt(). - Add routine for the additional conditions for 3rd order SSPLMM and correct step-size update in case the conditions are violated.
This looks ready to merge. It is great that now the 2nd-order methods allow for any number of steps and that there are tests for 4 different SSP LMMs. Nice work @hadjimy ! |
@mandli and @rjleveque PyClaw is ready for the 5.3 release. There is a small additional refactoring that we hope to get done in time, but if you want to generate a release candidate now, that is fine. |
This PR changes the step-size control for LMMs and revises the way the step-size is updated at each step for RK methods.
There is still some work currently in process to make the code more efficient and robust.
Note there is a test that fails in parallel (acoustics_2d_homogeneous) when SSPLMM32 is used, but could not reproduce that in my machine (where all tests pass).