-
Notifications
You must be signed in to change notification settings - Fork 255
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 21 commits
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
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
| InvalidOperator) | ||
| from devito.logger import (debug, info, perf, warning, is_log_enabled_for, | ||
| switch_log_level) | ||
| from devito.ir.equations import LoweredEq, lower_exprs, concretize_subdims | ||
| from devito.ir.equations import LoweredEq, lower_multistage, lower_exprs, concretize_subdims | ||
| from devito.ir.clusters import ClusterGroup, clusterize | ||
| from devito.ir.iet import (Callable, CInterface, EntryFunction, DeviceFunction, | ||
| FindSymbols, MetaCall, derive_parameters, iet_build) | ||
|
|
@@ -40,7 +40,6 @@ | |
| disk_layer) | ||
| from devito.types.dimension import Thickness | ||
|
|
||
|
|
||
|
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. Please run the linter (
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 |
||
| __all__ = ['Operator'] | ||
|
|
||
|
|
||
|
|
@@ -337,6 +336,8 @@ def _lower_exprs(cls, expressions, **kwargs): | |
| * Apply substitution rules; | ||
| * Shift indices for domain alignment. | ||
| """ | ||
| expressions = lower_multistage(expressions, **kwargs) | ||
|
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 should at least be called after and, perhaps, benefit from a more generic name such as
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. we could also move it inside 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. That makes sense, thanks! This was part of an earlier approach that after one meeting with Devito's team was decided to be left like a plan b, so it shouldn’t actually be here. I’ll remove it from the PR to appear only the actual approach—though I agree that structuring it that way would make sense if we revisit this idea in the future and I already changed accordingly.
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. Quick correction to my previous comment: I realized this part is actually still in use in the current implementation. I’ve updated it taking your suggestions into account (ordering + naming), so it should now reflect what it was intended. |
||
|
|
||
| expand = kwargs['options'].get('expand', True) | ||
|
|
||
| # Specialization is performed on unevaluated expressions | ||
|
|
||
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.
Is method a string here, or is it the class for the method? In the latter case, it would remove the need to have the
method_registrymapper. Furthermore, it would allow you to havemethod.resolve(target, sols_temp)here, which is tidierThere 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.
It's a string. The idea is that the user provides a string to identify which time integrator to apply.