-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Make fem::Form stateless #1168
Make fem::Form stateless #1168
Conversation
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 like the changes.
cpp/dolfinx/fem/Form.h
Outdated
FormIntegrals<T> _integrals; | ||
/// Set the valid domains for the integrals of a given type from a | ||
/// MeshTags "marker". Note the MeshTags is not stored, so if there | ||
/// any changes to the integration domain this must be called again. |
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.
"so if there are any changes..."?
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.
Create a new form . . .
std::shared_ptr<const mesh::Mesh> _mesh; | ||
}; | ||
|
||
using kern |
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.
Looks like this typedef could be made global somewhere now? It is heavily repeated.
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.
Let's discuss the separately. I often find that a heavily used typedef declared far from where it's used can reduce readability.
@@ -171,7 +171,7 @@ void _lift_bc_cells( | |||
|
|||
const std::function<void(T*, const T*, const T*, const double*, const int*, | |||
const std::uint8_t*, const std::uint32_t)>& fn | |||
= a.integrals().get_tabulate_tensor(IntegralType::cell, 0); | |||
= a.kernel(IntegralType::cell, -1); |
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 wonder why here the lifting happens only using default cell integral. Even worse, note that in the current master it used only first cell integral, not even the default one. I would say the lifting should add contributions from all cell integrals.
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 is an existing bug - see #1110.
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.
Ouch... Should be fixed with priority.
@@ -294,7 +292,7 @@ void _lift_bc_exterior_facets( | |||
|
|||
const std::function<void(T*, const T*, const T*, const double*, const int*, | |||
const std::uint8_t*, const std::uint32_t)>& fn | |||
= a.integrals().get_tabulate_tensor(IntegralType::exterior_facet, 0); | |||
= a.kernel(IntegralType::exterior_facet, -1); |
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.
Same wonder here.
…to garth/form-constructor
The change fully creates a usable Form object at construction.
The old design was an anti-pattern with incomplete construction, even requiring a class method to complete construction. Changes used maps rather than array with mapping arrays, which simplifies the code.
There are more possible changes to simplify the code, but the PR at least largely encapsulates them by making Form stateless.
Fixes #1164.