-
Notifications
You must be signed in to change notification settings - Fork 256
dsl: Introduce abstractions for multi-stage time integrators #2599
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
1d830b8
7f087b3
214d882
d6c4d4a
78f8a0b
1c9d517
11db48b
83dfb04
d47a106
1f93a45
eea3a52
11d1429
4637ac2
ac1da7e
e9b3533
dc3dd77
1fd4a02
a0c45c1
93c6e3f
ef8d1ac
fa5acac
143d0c2
5f67b91
552fd7f
e9d2000
f7c9ea3
cf1003c
1fd480b
a875224
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ | |
| from devito.symbolics import uxreplace | ||
| from numpy import number | ||
| from devito.types.array import Array | ||
| from devito.types.dense import Function | ||
| from devito.types.constant import Constant | ||
| from types import MappingProxyType | ||
|
|
||
| method_registry = {} | ||
|
|
@@ -51,7 +53,7 @@ class MultiStage(Eq): | |
| of update expressions for each stage in the integration process. | ||
| """ | ||
|
|
||
| def __new__(cls, lhs, rhs, **kwargs): | ||
| def __new__(cls, lhs, rhs, source=None, degree=6, **kwargs): | ||
| if not isinstance(lhs, list): | ||
| lhs=[lhs] | ||
| rhs=[rhs] | ||
|
|
@@ -61,6 +63,8 @@ def __new__(cls, lhs, rhs, **kwargs): | |
| obj._eq = [Eq(lhs[i], rhs[i]) for i in range(len(lhs))] | ||
| obj._lhs = lhs | ||
| obj._rhs = rhs | ||
| obj._deg = degree | ||
| obj._src = source | ||
|
|
||
| return obj | ||
|
|
||
|
|
@@ -79,6 +83,16 @@ def rhs(self): | |
| """Return list of right-hand sides.""" | ||
| return self._rhs | ||
|
|
||
| @property | ||
| def deg(self): | ||
| """Return list of right-hand sides.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For immutability, these should be
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| return self._deg | ||
|
|
||
| @property | ||
| def src(self): | ||
| """Return list of right-hand sides.""" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeated docstring?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| return self._src | ||
|
|
||
| def _evaluate(self, **kwargs): | ||
| raise NotImplementedError( | ||
| f"_evaluate() must be implemented in the subclass {self.__class__.__name__}") | ||
|
|
@@ -115,7 +129,9 @@ class RK(MultiStage): | |
| Number of stages in the RK method, inferred from `b`. | ||
| """ | ||
|
|
||
| def __init__(self, a: list[list[float | number]], b: list[float | number], c: list[float | number], lhs, rhs, **kwargs) -> None: | ||
| CoeffsBC = list[float | number] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, is the same, I changed it to np.number to avoid confusion |
||
| CoeffsA = list[CoeffsBC] | ||
| def __init__(self, a: CoeffsA, b: CoeffsBC, c: CoeffsBC, lhs, rhs, **kwargs) -> None: | ||
| self.a, self.b, self.c = a, b, c | ||
|
|
||
| @property | ||
|
|
@@ -132,19 +148,18 @@ def _evaluate(self, **kwargs): | |
|
|
||
| Returns | ||
| ------- | ||
| list of Eq | ||
| list of Devito Eq objects | ||
| A list of SymPy Eq objects representing: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: they will be Devito
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| - `s` stage equations of the form `k_i = rhs evaluated at intermediate state` | ||
| - 1 final update equation of the form `u.forward = u + dt * sum(b_i * k_i)` | ||
| """ | ||
| n_eq=len(self.eq) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs whitespace for flake8
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| u = [i.function for i in self.lhs] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making this a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did another thing where instead of having the |
||
| grid = [u[i].grid for i in range(n_eq)] | ||
| t = grid[0].time_dim | ||
| t = u[0].grid.time_dim | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. grids = {f.grid for f in u}
if not len(grids) == 1:
raise ValueError("Cannot construct multi-stage time integrator for Functions on disparate grids")
grid = grids.pop()
t = grid.time_dimwould be safer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'm wrong, but I think the functions should be allowed to be defined in different grids, like different staggered grids... the method should work also for those cases. |
||
| dt = t.spacing | ||
|
|
||
| # Create temporary Functions to hold each stage | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: these are
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right! |
||
| k = [[Array(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', dimensions=grid[j].dimensions, grid=grid[j], dtype=u[j].dtype) for i in range(self.s)] | ||
| k = [[Array(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', dimensions=u[j].grid.dimensions, grid=u[j].grid, dtype=u[j].dtype) for i in range(self.s)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. k = [[Array(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', dimensions=grid.dimensions, grid=grid, dtype=f.dtype) for f in u]or similar should be sufficient in combination with my previous comment
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even with that, all those combination are needed, because for each equation are required Arrays for all the Runge-Kutta stages. |
||
| for j in range(n_eq)] | ||
|
|
||
| stage_eqs = [] | ||
|
|
@@ -214,8 +229,8 @@ class RK32(RK): | |
| b = [0, 0, 1] | ||
| c = [0, 1/2, 1/2] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should definitely be tuple. Exposing a mutable data structure at the class level here is potentially dangerous
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(a=self.a, b=self.b, c=self.c, **kwargs) | ||
| def __init__(self, lhs, rhs, **kwargs): | ||
| super().__init__(a=self.a, b=self.b, c=self.c, lhs=lhs, rhs=rhs, **kwargs) | ||
|
|
||
|
|
||
| @register_method | ||
|
|
@@ -249,12 +264,12 @@ class RK97(RK): | |
| 5963949/25894400, 50000000000/599799373173, 28487/712800] | ||
| c = [0, 4/63, 2/21, 1/7, 7/17, 13/24, 7/9, 91/100, 1] | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(a=self.a, b=self.b, c=self.c, **kwargs) | ||
| def __init__(self, lhs, rhs, **kwargs): | ||
| super().__init__(a=self.a, b=self.b, c=self.c, lhs=lhs, rhs=rhs, **kwargs) | ||
|
|
||
|
|
||
| @register_method | ||
| class HORK(MultiStage): | ||
| class HORK_EXP(MultiStage): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Be explicit! Use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also if we want to start distinguishing between explicit and implicit timesteppers we should be using a class hierarchy to do so
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, changed it. Actually, the EXP was for Exponential, not Explicit. But I agree that if/when implicit method are implemented, a class hierarchy is the best option. |
||
| # In construction | ||
| """ | ||
| n stages Runge-Kutta (HORK) time integration method. | ||
|
|
@@ -271,8 +286,19 @@ class HORK(MultiStage): | |
| Time positions of intermediate stages. | ||
| """ | ||
|
|
||
| def source_derivatives(self, src_index, t, **kwargs): | ||
|
|
||
| # Compute the base wavelet function | ||
| f_deriv = [[self.src[i][1] for i in range(len(self.src))]] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have a couple of one-liners like this where you use f_deriv = [[src[1] for src in self.src]]You might want to go through your PR and tidy them up
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I went through the PR and changed a few of the range(len()) inside the loops |
||
|
|
||
| # Compute derivatives up to order p | ||
| for _ in range(self.deg - 1): | ||
| f_deriv.append([f_deriv[-1][i].diff(t) for i in range(len(src_index))]) | ||
|
|
||
| f_deriv.reverse() | ||
|
EdCaunt marked this conversation as resolved.
|
||
| return f_deriv | ||
|
|
||
| def ssprk_alpha(mu=1, **kwargs): | ||
| def ssprk_alpha(self, mu=1): | ||
| """ | ||
| Computes the coefficients for the Strong Stability Preserving Runge-Kutta (SSPRK) method. | ||
|
|
||
|
|
@@ -287,18 +313,33 @@ def ssprk_alpha(mu=1, **kwargs): | |
| numpy.ndarray | ||
| Array of SSPRK coefficients. | ||
| """ | ||
| degree=kwargs.get('degree') | ||
|
|
||
| alpha = [0]*degree | ||
| alpha = [0]*self.deg | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could do that, but I thought that in cases like this is was better to use list. Isn't? |
||
| alpha[0] = 1.0 # Initial coefficient | ||
|
|
||
| for i in range(1, degree): | ||
| alpha[i] = 1 / (mu * (i + 1)) * alpha[i - 1] | ||
| alpha[1:i] = 1 / (mu * list(range(1, i))) * alpha[:i - 1] | ||
| for i in range(1, self.deg): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Some comments would help here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some comments, tell me if that is what you had in mind...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All looks reasonable |
||
| alpha[i] = 1/(mu*(i+1))*alpha[i-1] | ||
| alpha[1:i] = [1/(mu*j)*alpha[j-1] for j in range(1,i)] | ||
| alpha[0] = 1 - sum(alpha[1:i + 1]) | ||
|
|
||
| return alpha | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't look to be an array getting returned
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
|
|
||
|
|
||
| def source_inclusion(self, u, k, src_index, src_deriv, e_p, t, dt, mu, n_eq): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function could also use a few more comments. Additionally, is there any way to reduce the number of args? Possibly some of these args should be properties of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did something in this direction... |
||
|
|
||
| src_lhs = [uxreplace(self.rhs[i], {u[m]: k[m] for m in range(n_eq)}) for i in range(n_eq)] | ||
|
|
||
| p = len(src_deriv) | ||
|
|
||
| for i in range(p): | ||
| if e_p[i] != 0: | ||
| for j in range(len(src_index)): | ||
| src_lhs[src_index[j]] += self.src[j][0]*src_deriv[i][j].subs({t: t * dt})*e_p[i] | ||
| e_p = [e_p[i]+mu*dt*e_p[i + 1] for i in range(p - 1)]+[e_p[-1]] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would append
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but if I do that, |
||
|
|
||
| return src_lhs, e_p | ||
|
|
||
|
|
||
| def _evaluate(self, **kwargs): | ||
| """ | ||
| Generate the stage-wise equations for a Runge-Kutta time integration method. | ||
|
|
@@ -315,66 +356,52 @@ def _evaluate(self, **kwargs): | |
| - 1 final update equation of the form `u.forward = u + dt * sum(b_i * k_i)` | ||
| """ | ||
|
|
||
| u = self.lhs.function | ||
| rhs = self.rhs | ||
| grid = u.grid | ||
| t = grid.time_dim | ||
| n_eq=len(self.eq) | ||
| u = [i.function for i in self.lhs] | ||
| t = u[0].grid.time_dim | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the tweaks to this in my earlier comment. If it keeps popping up in different methods, then it should probably be a property of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hummm, I get your point, in this case, |
||
| dt = t.spacing | ||
|
|
||
| an_eq = range(len(U0)) | ||
| # Create a temporary Array for each variable to save the time stages | ||
| # k = [Array(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', dimensions=u[i].grid.dimensions, grid=u[i].grid, dtype=u[i].dtype) for i in range(n_eq)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is to be Related to the other comment, I think I could define the Arrays earlier and pass them as arguments. Is that what you have in mind? |
||
| k = [Function(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', grid=u[i].grid, space_order=2, time_order=1, dtype=u[i].dtype) for i in range(n_eq)] | ||
| k_old = [Function(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', grid=u[i].grid, space_order=2, time_order=1, dtype=u[i].dtype) for i in range(n_eq)] | ||
|
|
||
| # Compute SSPRK coefficients | ||
| alpha = np.array(ssprk_alpha(mu, degree), dtype=np.float64) | ||
| mu = 1 | ||
| alpha = self.ssprk_alpha(mu=mu) | ||
|
EdCaunt marked this conversation as resolved.
|
||
|
|
||
| # Initialize symbolic differentiation for source terms | ||
| t_var = sym.Symbol('t_var') | ||
| src_deriv = aux_fun.derivates_f(degree, f0) | ||
| src_index_map={val: i for i, val in enumerate(u)} | ||
| src_index = [src_index_map[val] for val in [self.src[i][2] for i in range(len(self.src))]] | ||
| src_deriv = self.source_derivatives(src_index, t, **kwargs) | ||
|
|
||
| # Expansion coefficients for stability control | ||
| e_p = [0] * degree | ||
| e_p = [0] * self.deg | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could do that, but I thought that in cases like this is was better to use list. Isn't?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably fine then |
||
| eta = 1 | ||
| e_p[-1] = 1 / eta | ||
|
|
||
| # Initialize approximation and auxiliary variable | ||
| approx = [U0[i] * alpha[0] for i in n_eq] | ||
| aux = U0 | ||
|
|
||
| # Perform Runge-Kutta steps | ||
| for i in range(1, degree - 1): | ||
| system_op, e_p = sys_op_extended(aux, x, y, z, param_fun, system, fd_order, src_spat, src_deriv, t, dt, t_var, e_p) | ||
| aux = [aux[j] + mu * dt * system_op[j] for j in n_eq] | ||
| approx = [approx[j] + aux[j] * alpha[i] for j in n_eq] | ||
|
|
||
| # Final Runge-Kutta updates | ||
| system_op, e_p = sys_op_extended(aux, x, y, z, param_fun, system, fd_order, src_spat, src_deriv, t, dt, t_var, e_p) | ||
| aux = [aux[i] + mu * dt * system_op[i] for i in n_eq] | ||
| system_op, e_p = sys_op_extended(aux, x, y, z, param_fun, system, fd_order, src_spat, src_deriv, t, dt, t_var, e_p) | ||
| aux = [aux[i] + mu * dt * system_op[i] for i in n_eq] | ||
|
|
||
| # Compute final approximation | ||
| approx = [approx[i] + aux[i] * alpha[degree - 1] for i in n_eq] | ||
|
|
||
| # Generate final PDE system | ||
| return [dv.Eq(U0[i].forward, approx[i]) for i in n_eq] | ||
|
|
||
| # Create temporary Functions to hold each stage | ||
| # k = [Array(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', dimensions=grid.shape, grid=grid, dtype=u.dtype) for i in range(self.s)] # Trying Array | ||
| k = [Function(name=f'{kwargs.get('sregistry').make_name(prefix='k')}', grid=grid, space_order=u.space_order, dtype=u.dtype) | ||
| for i in range(self.s)] | ||
|
|
||
| stage_eqs = [] | ||
| stage_eqs = [Eq(k[i], u[i]) for i in range(n_eq)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be tidier using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| [stage_eqs.append(Eq(u[i].forward, u[i]*alpha[0])) for i in range(n_eq)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't love this one-lining. Make this an explicit for loop, or use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| # Build each stage | ||
| for i in range(self.s): | ||
| u_temp = u + dt * sum(aij * kj for aij, kj in zip(self.a[i][:i], k[:i])) | ||
| t_shift = t + self.c[i] * dt | ||
| for i in range(1, self.deg-1): | ||
| [stage_eqs.append(Eq(k_old[j], k[j])) for j in range(n_eq)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto, purge this style of one-lining throughout, and use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| src_lhs, e_p = self.source_inclusion(u, k_old, src_index, src_deriv, e_p, t, dt, mu, n_eq) | ||
| [stage_eqs.append(Eq(k[j], k_old[j]+mu*dt*src_lhs[j])) for j in range(n_eq)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of these comprehensions should use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| [stage_eqs.append(Eq(u[j].forward, u[j].forward+k[j]*alpha[i])) for j in range(n_eq)] | ||
|
|
||
| # Evaluate RHS at intermediate value | ||
| stage_rhs = uxreplace(rhs, {u: u_temp, t: t_shift}) | ||
| stage_eqs.append(Eq(k[i], stage_rhs)) | ||
| # Final Runge-Kutta updates | ||
| [stage_eqs.append(Eq(k_old[j], k[j])) for j in range(n_eq)] | ||
| src_lhs, e_p = self.source_inclusion(u, k_old, src_index, src_deriv, e_p, t, dt, mu, n_eq) | ||
| [stage_eqs.append(Eq(k[j], k_old[j]+mu*dt*src_lhs[j])) for j in range(n_eq)] | ||
|
|
||
| # Final update: u.forward = u + dt * sum(b_i * k_i) | ||
| u_next = u + dt * sum(bi * ki for bi, ki in zip(self.b, k)) | ||
| stage_eqs.append(Eq(u.forward, u_next)) | ||
| [stage_eqs.append(Eq(k_old[j], k[j])) for j in range(n_eq)] | ||
| src_lhs, _ = self.source_inclusion(u, k_old, src_index, src_deriv, e_p, t, dt, mu, n_eq) | ||
| [stage_eqs.append(Eq(k[j], k_old[j]+mu*dt*src_lhs[j])) for j in range(n_eq)] | ||
|
|
||
| # Compute final approximation | ||
| [stage_eqs.append(Eq(u[j].forward, u[j].forward+k[j]*alpha[self.deg-1])) for j in range(n_eq)] | ||
|
|
||
| return stage_eqs | ||
|
|
||
|
|
||
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 file should be moved to somewhere like
devito/timestepping/rungekutta.pyordevito/timestepping/explicitmultistage.pythat way additional timesteppers can be contributed as new files. (I'm thinking about implicit multistage, backward difference formulae etc...)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 renamed the class to
HighOrderRungeKuttaExponential. I realize the name might be confusing since this particular Runge-Kutta is explicit, but “EXP” was intended to highlight the exponential aspect. I’ve also updated the other class names based on your suggestions.Regarding the file location, it’s currently in
/typesas recommended by @mloubout (see suggestion). Personally, I think both/timesteppingand/typesare reasonable options. Perhaps we can discuss this with @EdCaunt and @FabioLuporini to reach a consensus.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 agree this file doesn't belong to
types/based on https://github.com/devitocodes/devito/pull/2599/changes#r3043562368, we might add it to
ir/dsl/rungekutta.py