Make reuse of mapping data in FEPointEvaluation safer #13245
Conversation
There was a problem hiding this comment.
This is a good step forward but I must I didn't get all the details.
As far as I understand, we have mapping data that is shared by different FPEs. Furthermore, each of them can modify it, which leads to some issues as pointed out in my comments. @kronbichler Could you comment if I understood the code correctly. If I see it correctly, the biggest obstacle to make the mapping data immutable are the mapping flags, that we cannot pre-collect.
| * @p cell and @p unit_points that is required by the FEPointEvaluation | ||
| * class. | ||
| * Data structure that allows the computation of the mapping related data | ||
| * for the given @p mapping, @p cell and @p unit_points that us used by |
There was a problem hiding this comment.
| * for the given @p mapping, @p cell and @p unit_points that us used by | |
| * for the given @p mapping, @p cell and @p unit_points that are used by |
| /** | ||
| * The points for which the evaluation has been invoked by the last call | ||
| * to compute(). | ||
| */ | ||
| std::vector<Point<dim>> unit_points; | ||
|
|
||
| /** | ||
| * The cell on which the evaluation has been invoked by the last call | ||
| * to compute(). | ||
| */ | ||
| const typename Triangulation<dim, spacedim>::cell_iterator cell; | ||
|
|
||
| /** | ||
| * The data computed by this data structure. | ||
| */ | ||
| internal::FEValuesImplementation::MappingRelatedData<dim, spacedim> data; | ||
| }; |
There was a problem hiding this comment.
Do we really have these public? As far as I see, the instance of this class returned by get_mapping_data() is mutable.
| bool needs_update = | ||
| (cell != this->cell) || unit_points.size() != this->unit_points.size(); | ||
| // expensive check: make sure all points are really the same | ||
| if (!needs_update) | ||
| for (unsigned int i = 0; i < unit_points.size(); ++i) | ||
| if (unit_points[i].distance_square(this->unit_points[i]) > 1e-12) | ||
| { | ||
| needs_update = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I guess this check is needed, because all FPE using the same mapping instance can modify it. Wouldn't it be better if there is a master FPE: this is updates the mapping info and returns a non-mutable mapping instance. Also, I am not completely sure about the two reinit functions:
reinit(cell, points) -> void
reinit(cell, points, mapping) -> void
As far as I understand from the documentation, the mapping object in the second reinit function already contains the precomputed mapping. That means we do not need cell and points in that case because it is already provided by the mapping object, isn't it? Or this mapping object filled (like in VT::point_values()) - in that case the documentation is out of date.
There was a problem hiding this comment.
Wouldn't it be better if there is a master FPE: this is updates the mapping info and returns a non-mutable mapping instance.
I agree with @peterrum. It is a nice thought to share the mapping data instead of creating copies (like the code does at the moment), but the shared data should be const, otherwise modifying a later FPE might change the behavior of the original one.
As far as I understand from the documentation, the mapping object in the second reinit function already contains the precomputed mapping.
That is correct. At the moment the second function assumes (and checks) that the mapping data has already been computed, handing over an empty object is not allowed.
| // As we got the data from the other field, we will typically not need | ||
| // to perform any operation, unless the user provided us with a | ||
| // `precomputed_mapping_data` that is not initialized to the present | ||
| // context. | ||
| mapping_data->compute(*mapping, cell, unit_points, update_flags_mapping); |
There was a problem hiding this comment.
OK. The flags might be different between FPEs that share the same mapping (additional fields are filled). But if cell and unit_points is different, this looks like a bug in the code. Also, does that mean that we are modifying the internal state of the other FPE that has called this function before? We don't have any way to signal that its that has been invalidated.
There was a problem hiding this comment.
I agree with Peter's concerns. It is a nice addition to hide the internal type and to share the data between FPE's (instead of copying them), but we should not change the shared structure. I see that making it const is impractical when different FPE's need different mapping data (like jacobians), but maybe that is simply the limitation? If you need different mapping data for different FPE's you should not try to reuse their mapping data? Not sure how severe this limitation is in practice.
| std::vector<Point<dim>> unit_points; | ||
|
|
||
| /** | ||
| * The cell on which the evaluation has been invoked by the last call |
There was a problem hiding this comment.
| * The cell on which the evaluation has been invoked by the last call | |
| * The cell on which the evaluation has been invoked by the last call |
| bool needs_update = | ||
| (cell != this->cell) || unit_points.size() != this->unit_points.size(); | ||
| // expensive check: make sure all points are really the same | ||
| if (!needs_update) | ||
| for (unsigned int i = 0; i < unit_points.size(); ++i) | ||
| if (unit_points[i].distance_square(this->unit_points[i]) > 1e-12) | ||
| { | ||
| needs_update = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better if there is a master FPE: this is updates the mapping info and returns a non-mutable mapping instance.
I agree with @peterrum. It is a nice thought to share the mapping data instead of creating copies (like the code does at the moment), but the shared data should be const, otherwise modifying a later FPE might change the behavior of the original one.
As far as I understand from the documentation, the mapping object in the second reinit function already contains the precomputed mapping.
That is correct. At the moment the second function assumes (and checks) that the mapping data has already been computed, handing over an empty object is not allowed.
|
@kronbichler How do we want to proceed here? |
|
Just for the record, @bergbauer is working on a better replacement of this. Once we have the code posted (hopefully early next week), we can close this PR and thus remove it from the 9.4 milestone target, but before I would like to keep it on the milestone to have the awareness that something needs to be done now before the release. |
|
Closed in favor of #13835. |
Follow-up to #12983: As discussed #12983 (comment), we should make sure that a user cannot mix up the order by which to provide the information. This PR achieves this by making the data structure a shared pointer of some internal struct that also remembers the cell which it was last initialized to. This makes sure we can update the data on demand.