Fix do_reinit of FEPointEvaluation for empty unit_points #15176
Conversation
|
I don't know the code in question, but would appreciate quick resolution -- |
bergbauer
force-pushed
the
fix_empty_quadrature_fe_point
branch
from
702793e
to
e9feb6a
Compare
|
|
||
| if (n_q_points == 0) | ||
| { | ||
| is_reinitialized = true; |
There was a problem hiding this comment.
The early return looks not correct. All the pointers should be set nullptr.
There was a problem hiding this comment.
You are right, they should be set nullptr and then we return early.
bergbauer
force-pushed
the
fix_empty_quadrature_fe_point
branch
from
e9feb6a
to
3a62eb3
Compare
| unit_point_ptr = nullptr; | ||
| real_point_ptr = nullptr; | ||
| jacobian_ptr = nullptr; | ||
| inverse_jacobian_ptr = nullptr; | ||
| normal_ptr = nullptr; | ||
| JxW_ptr = nullptr; |
There was a problem hiding this comment.
It's not wrong, but I don't understand why these are needed from a correctness point of view. As far as I can see, all uses of these variables in the context of nullptr in this class can they are all after an AssertIndexRange(point_index, n_q_points); - in other words, while the pointers might not point to the right place, it won't hurt too much, and in fact I would think we then need to set them to nullptr also in the respective constructor.
There was a problem hiding this comment.
You are right the assert should catch all cases. Should we remove setting nullptr again? We should keep the resizing of values and gradient field or we also add the assert AssertIndexRange(point_index, n_q_points); there.
There was a problem hiding this comment.
I have to leave at 7 pm today so we should converge to a final solution. @peterrum @kronbichler
There was a problem hiding this comment.
I have removed setting nullptr and kept the resizing. ready from my side. We can discuss the optimal solution in a follow-up, this is should be sufficient to fix master.
bergbauer
force-pushed
the
fix_empty_quadrature_fe_point
branch
from
3a62eb3
to
93d2903
Compare
|
OK from my side. |
| (n_q_points_scalar + n_lanes_user_interface - 1) / n_lanes_user_interface; | ||
|
|
||
| if (update_flags & update_values) | ||
| values.resize(n_q_points, numbers::signaling_nan<value_type>()); |
There was a problem hiding this comment.
resize() and not assign is wanted? It feels odd that only the new entries are set to nan.
There was a problem hiding this comment.
If at all possible, I would avoid using assign at least in release mode, because these operations are not for free. When resizing, we have no other choice, but for the remaining entries we should not.
|
/rebuild |
Fixes https://ci.tjhei.info/job/dealii-serial/job/master/lastBuild/testReport/projectroot[…]ching/mapping_info_02debug/non_matching_mapping_info_02_debug/
FYI @peterrum @fdrmrc @kronbichler