-
Notifications
You must be signed in to change notification settings - Fork 231
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
dsl: Introduce ability to define Functions on Subdomains #2245
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2245 +/- ##
===========================================
- Coverage 87.30% 44.92% -42.38%
===========================================
Files 238 238
Lines 45972 46877 +905
Branches 4074 4134 +60
===========================================
- Hits 40134 21061 -19073
- Misses 5150 24926 +19776
- Partials 688 890 +202 ☔ View full report in Codecov by Sentry. |
5fc54ee
to
a0742f0
Compare
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
9fe8916
to
1092e78
Compare
1d37846
to
d462af1
Compare
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.
For the amount of changes introduced, I'd prefer to see a lot more new tests.
Shouldn't all (most) of the old tests that @rhodrin wrote be here as well?
588c25e
to
2cd952e
Compare
2cd952e
to
341ee08
Compare
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.
quick pass
4268c54
to
af06ba9
Compare
416bb4a
to
a67dd12
Compare
@@ -230,6 +230,10 @@ | |||
"cell_type": "markdown", | |||
"metadata": {}, | |||
"source": [ | |||
"The Devito `SubDomain` API features two steps. The first of these is to create a subclass of `SubDomain`, which encapsulates the template by which `SubDomain`s should be constructed. This is not tied to a specific `Grid`. On a basic level, this specification is created by defining a suitable `define` method for the subclass.\n", | |||
"\n", | |||
"Note if one intends to introduce a custom `__init__` when subclassing `SubDomain`, then it is necessary to provide the `kwarg` `grid=None`, and pass this through to `super().__init__()`. Such a custom `__init__` may be useful in applications such as creating many `SubDomain` instances with similar properties from a single template.\n", |
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.
this seems to kind of contradict the paragraph before...
@@ -339,17 +312,28 @@ | |||
"\n", | |||
"The two other options available are `'left'` and `'right'`. For a statement of the form `d: ('left', N)` the `SubDomain` spans a contiguous region of `N` points starting at `d`\\'s left extreme. A statement of the form `('right', N)` is analogous to the previous case but starting at the dimensions right extreme instead.\n", | |||
"\n", | |||
"Now, to create a grid containing both of the subdomains defined above, 'mydomain' and 'inner', we simply pass these during the creation of a grid:" | |||
"We can also attach this subdomain to the grid." |
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.
"We can" or "We must" ("we have to") since we actually want to use it?
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.
Now that the notebook updates are in, I think we also need:
- either a short, additional notebook
examples/userapi/03_subdomains_legacy_API.ipynb
- or a section in
examples/userapi/03_subdomains.ipynb
explaining the legacy API
Add Functions on SubDomains functionality.
Remaining todo (for future PRs I think):