Increment form for implicit RK added and tested #566
Conversation
tommbendall
added
the
enhancement
| @@ -56,6 +57,7 @@ class ImplicitRungeKutta(TimeDiscretisation): | |||
| # --------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Is it worth us making the predictor and increment forms clear in the docstrings?
There was a problem hiding this comment.
I've made it a bit more clear, describing what we are solving for
| else: | ||
| raise NotImplementedError( | ||
| 'Runge-Kutta formulation is not implemented' | ||
| ) | ||
|
|
||
| def lhs(self): |
There was a problem hiding this comment.
Do you know we don't set up the lhs and rhs variables? If these are inherited from the base TimeDiscretisation class, are they wrong? They don't appear to be used anywhere either. I wonder if we should either:
- use them
- set their values to be
None - remove them and stop them being an
@abstractproperty
There was a problem hiding this comment.
I have removed lhs & rhs and stopped them being an abstract property
There was a problem hiding this comment.
Instead they are now just a property
There was a problem hiding this comment.
Thanks for making this change. @jshipton are you happy with this?
| map_if_false=replace_subject(self.xnph, self.idx), | ||
| ) | ||
| def res(self, stage): | ||
| """Set up the discretisation's residual for a given stage.""" |
There was a problem hiding this comment.
Can we make clear that this is only for the predictor formulation?
| @property | ||
| def final_res(self): | ||
| """Set up the discretisation's final residual.""" | ||
| # Add time derivative terms y^{n+1} - y^n |
There was a problem hiding this comment.
Can we make clear that this is only for the predictor formulation?
There was a problem hiding this comment.
Two minor suggestions from me. I'm happy but think it would also be helpful for @jshipton have a look
|
|
||
| @cached_property | ||
| def final_solver(self): | ||
| """Set up a solver for the final solve for the predictor formulation to evaluate time level n+1.""" |
There was a problem hiding this comment.
Can you split this comment over multiple lines?
Non-increment form for Implicit RK does not yet exist. This is likely required for running implicit RK with compressible Euler equations, so this capability has been added (as in explicit rk)