FEEval: do not cast const away during read_dof_values() #13240
Conversation
| const_cast<typename internal::BlockVectorSelector< | ||
| typename std::remove_const<VectorType>::type, | ||
| IsBlockVector<typename std::remove_const<VectorType>::type>::value>:: | ||
| BaseVectorType &>(*src_data.first[d]), |
There was a problem hiding this comment.
We should also remove this const_cast but this is more work. Also in that code path there are no asserts of this type.
| @@ -3270,6 +3270,22 @@ FEEvaluationBase<dim, n_components_, Number, is_face, VectorizedArrayType>:: | |||
|
|
|||
| namespace internal | |||
| { | |||
| template <typename VectorType, bool> | |||
| struct ConstBlockVectorSelector | |||
| {}; | |||
There was a problem hiding this comment.
You might as well remove the whole body of this class and just provide a forward declaration since there is no case you are not covering via a specialization.
There was a problem hiding this comment.
Thanks for the review. I have removed the body (I just did it how it was below).
peterrum
force-pushed
the
fe_eval_const_gather
branch
from
4a44987
to
cd3fee6
Compare
|
/rebuild |
| #ifdef DEBUG | ||
| for (unsigned int i = 0; i < dofs_per_cell; ++i) | ||
| for (unsigned int v = 0; v < VectorizedArrayType::size(); ++v) | ||
| vector_access(vec, dof_index + v + i * VectorizedArrayType::size()); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This is at least surprising for someone reading the code because you load the vector twice. I would prefer to do not do the load part below in debug mode (i.e., use an #if / #else branch) or at the very least a extensive comment. The former assumes we have other paths where we test VectorizedArray::load.
There was a problem hiding this comment.
I have added the #if/#else structure and added some comments.
| #ifdef DEBUG | ||
| for (unsigned int v = 0; v < VectorizedArrayType::size(); ++v) | ||
| vector_access(vec, indices[v] + constant_offset); | ||
| #endif |
There was a problem hiding this comment.
Same here, I'd prefer an #if/#else statement to avoid confusion about doing the same thing twice.
Just for my understanding, you had a vector that has the right ghost elements in the partitioner (i.e., the partitioners match) but has not called dealii/include/deal.II/lac/la_parallel_vector.h Lines 1727 to 1729 in 3447d0a |
Yes. Everything correct, just that |
peterrum
force-pushed
the
fe_eval_const_gather
branch
from
cd3fee6
to
a6ec42c
Compare
This was an interesting "bug". I have passed to
FEEval::read_dof_values()a clearly not ghosted vector and did not get any assert. The reason is that we (maybe me) do cast away the constness of the vector and when callingLA::d:V::local_element()we do not get the assert which is only available in the const version of this function.