Increment form for implicit RK added and tested #566
Conversation
tommbendall
left a comment
tommbendall
left a comment
There was a problem hiding this comment.
A few suggested changes, but otherwise the tests convince me that the formulae are probably correct
| # [a_10 a_11 . 0 ] | ||
| # [ . . . . ] | ||
| # [ b_0 b_1 . b_s] | ||
| # --------------------------------------------------------------------------- |
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
| '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?
There was a problem hiding this comment.
I have now removed all lhs & rhs. Each time discretisation just has a res (residual).
Co-authored-by: Thomas Bendall <thomas.bendall@metoffice.gov.uk>
tommbendall
left a comment
tommbendall
left a comment
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
… in results or timings
…fference in results or timings" This reverts commit 249dfd3.
Co-authored-by: Thomas Bendall <thomas.bendall@metoffice.gov.uk>
tommbendall
left a comment
tommbendall
left a comment
There was a problem hiding this comment.
Great, thanks Alex!!
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)