Skip to content
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

Matrix-free Piola transformation for affine cells #13642

Merged
merged 5 commits into from May 23, 2022

Conversation

NiklasWik
Copy link
Contributor

@NiklasWik NiklasWik commented Apr 25, 2022

This PR builds on #13591 and adds the Piola transformation for affine cells when using the matrix-free evaluation of the FE_RaviartThomasNodal element. It also updates the tests associated with the previous pr (with ideas from @peterrum) and adds two new ones for non-Cartesian cells.

( #9545)

@peterrum peterrum self-requested a review April 25, 2022 20:51
@peterrum
Copy link
Member

@NiklasWik I'll take a look at this PR once the other PR is merged and you have rebased this one!

@NiklasWik NiklasWik force-pushed the piola_matrix_free branch 2 times, most recently from 7f770f4 to 4632283 Compare April 27, 2022 08:35
{
const Tensor<2, dim, VectorizedArrayType> &inv_t_jac =
this->jacobian[0];
const Tensor<2, dim, VectorizedArrayType> &t_jac = invert(inv_t_jac);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not documented anywhere, but you do not need to compute inverse matrix for Cartesian/affine cells

Suggested change
const Tensor<2, dim, VectorizedArrayType> &t_jac = invert(inv_t_jac);
const Tensor<2, dim, VectorizedArrayType> &t_jac = this->jacobian[1];

This is filled in here for cells:

for (unsigned int d = 0; d < dim; ++d)
for (unsigned int e = 0; e < dim; ++e)
store_vectorized_array(
jac[d][e],
vv,
my_data.jacobians[0][idx + 1][d][e]);
. Note that this is not done for faces (if I remember correctly) but we could do it if necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! It does seem unnecessary to compute it twice... I'll have a look at it tomorrow (and also update the documentation)

Copy link
Contributor Author

@NiklasWik NiklasWik Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find that this is used anywhere in matrix_free/fe_evaluation.h but I'm unsure if it is used anywhere else. Because I would like to change it to the transpose jacobian instead of just the jacobian. That way there's no need to call transpose(invert(inv_t_jac)) if the cell is general in for instance get_values. I would also add it for faces.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use is here:

if (is_face == false &&
this->cell_type <= internal::MatrixFreeFunctions::affine)
{
Assert(this->jacobian != nullptr, ExcNotInitialized());
Point<dim, Number> point = this->quadrature_points[0];
const Tensor<2, dim, Number> &jac = this->jacobian[1];
if (this->cell_type == internal::MatrixFreeFunctions::cartesian)
for (unsigned int d = 0; d < dim; ++d)
point[d] += jac[d][d] * static_cast<typename Number::value_type>(
this->descriptor->quadrature.point(q)[d]);
else
for (unsigned int d = 0; d < dim; ++d)
for (unsigned int e = 0; e < dim; ++e)
point[d] += jac[d][e] * static_cast<typename Number::value_type>(
this->descriptor->quadrature.point(q)[e]);
return point;

Regarding a call to transpose, I would expect that an optimizing compiler optimizes the memory operations away. (It does not always work, but I've seen it work often.) Hence, it should not matter whether you call transpose(jac). Note that for general cells the Jacobians are not stored, just for the affine/Cartesian case (as the latter would be too memory intensive).

Comment on lines 32 to 45
template void DoFTools::make_periodicity_constraints(
const DoFHandler<deal_II_dimension,
deal_II_space_dimension>::face_iterator &,
const DoFHandler<deal_II_dimension,
deal_II_space_dimension>::face_iterator &,
AffineConstraints<float> &,
const ComponentMask &,
bool,
bool,
bool,
const FullMatrix<double> &,
const std::vector<unsigned int> &,
const float);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change in the number of instantiations and the one a few lines below should be in a separate PR. Would you mind opening that one so it can be reviewed separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree!

@NiklasWik
Copy link
Contributor Author

I ended up changing to the transpose jacobian anyway because it simplifies the code a bit. Also added it to the faces.

All matrixfree tests pass.

@NiklasWik
Copy link
Contributor Author

NiklasWik commented Apr 28, 2022

I guess it is used somewhere else as well. I'll change it back, but the code will get a bit uglier. :/

@kronbichler
Copy link
Member

/rebuild

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NiklasWik Let me post what I have till now. I will continue with include/deal.II/matrix_free/fe_evaluation.h later today.

Comment on lines 16 to 21
// This function tests the correctness of the matrix-free implementation
// of the FE_RaviartThomasNodal element by evaluating values + gradients
// as well as the divergence on faces and comparing the result with
// FEFaceVaules which is considered the reference. The mesh is a
// subdivided_parallelepiped with no hanging nodes and no other constraints,
// i.e affine but not cartesian.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could shorten the comment here by referring to the other test and only state what is different (i.e., non-Cartesian).

@@ -61,7 +61,38 @@ template <int dim, int fe_degree>
void
test();

enum TestType : unsigned char
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

Comment on lines +72 to +73
std::string
enum_to_string(TestType const enum_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit over-engineered :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just grabbed it from exadg I think

my_data.jacobians[0][idx + 1][d][e]);
my_data.jacobians[0][idx + 1][e][d]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug? If yes, could you extract it to separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not! It has to do with the discussion above with storing the transpose or not. Not sure if this is what breaks the tests or not. I made a change in include/deal.II/matrix_free/fe_evaluation_data.h where this is used.

Comment on lines +2186 to +2185
const unsigned int ee =
ExtractFaceHelper::reorder_face_derivative_indices<
dim>(interior_face_no, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need here to pass in the reference-cell type. See other occurrences. Maybe we should remove the default parameter there so that such a mistake does not happen again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that I didn't have to since it is not done on line 2172. Is it a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it is. I wasn't sure which one of the two that was being used. Looking at it again makes it clear that I don't need this part, but only the same thing I did a couple of lines above. Since this is not used by the piola transform, and not anywhere else, I see no point in keeping it. I can keep it though, for consistency, and just remove the reorder_face_derivative_indices since it essentially does not do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I mean it does Assert(index < dim, ExcInternalError()); so I guess it has some value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually only used in 2 out of 12 places with reference-cell passed in. And what I said above is not correct. Sorry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just see this is the MappingQ path. Simplex meshes and mixed meshes should not enter this path; so you don't need to pass in the extra argument. Sorry for the confusion.

{
template <int dim, bool is_face>
inline const unsigned int *
get_index(const std::uint8_t fn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is fn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Face number

{
template <int dim, bool is_face>
inline const unsigned int *
get_index(const std::uint8_t fn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this function do? Why does it return a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it to get the right indicies for faces, i.e the inverse of reorder_face_derivative_indices

Comment on lines 5728 to 5731
static const unsigned int index0[3] = {0, 1, 2};
static const unsigned int index1[3] = {1, 0, 2};
static const unsigned int index2[3] = {2, 0, 1};
static const unsigned int index3[3] = {1, 2, 0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets' try not use C-arrays and rather use std::array? I guess what you want here is 2D array? You can check out ndarray.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really want to call this function more than once, and I figured that I only wanted to return a 1D array. But maybe I should do like what is done in reorder_face_derivative_indices, and call it for every index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that a static std::array should not be all that different from the pointer to a 1D array, as long as the compiler inlines everything correctly. I can't say whether it makes sense to call a function similar to reorder_face_derivative_indices for every index without benchmarking or looking at the assembly output of a compiler, as the function here is different from the one in MappingInfo in the sense that this one here is a hot loop, whereas the other function is "only" a setup function and thus potentially not used as often. That was also the intent for the original design by @kkormann and me, we tried to move everything expensive like shuffling around indices to the setup function to have as simple data structures in the actual operator evaluation for the regular DG face integrals.

Obviously, here we need to apply the reordering for every point in the evaluation. Or do we? Right now it is only curiosity, but do you think one could simplify the evaluators by shuffling around the normal and tangential components of the solution in such a way that the re-ordering of the Jacobian matrix on the faces would be the correct one again? I could even imagine that this might somewhat reduce the number of cases in the evaluation_kernels for the faces again, at least in terms of polynomial degrees that need to be expanded in normal and tangential direction. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think you are correct! I see no immediate reason why we wouldn't be able to shuffle the components like we shuffle the derivatives in gradients_quad for instance, which is why we don't have to shuffle the inverse jacobian in the get/submit functions. I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fyi, it really made the function in evaluation_kernels muuuch nicer! Great idea @kronbichler, thanks! :)

Now I also (finally) see the value of having zx-coordinates for the faces

Comment on lines +1008 to +1012
/**
* @copydoc FEEvaluationBase<dim,dim,Number,is_face>::get_value()
*/
value_type
get_value(const unsigned int q_point) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to make similar to what is done for get_gradient()?

Comment on lines 6407 to 6412
if (this->data->element_type ==
internal::MatrixFreeFunctions::ElementType::tensor_raviart_thomas)
{
// TODO
AssertThrow(false, ExcNotImplemented());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this->data->element_type ==
internal::MatrixFreeFunctions::ElementType::tensor_raviart_thomas)
{
// TODO
AssertThrow(false, ExcNotImplemented());
}
AssertThrow(this->data->element_type !=
internal::MatrixFreeFunctions::ElementType::tensor_raviart_thomas, ExcNotImplemented());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if-branch will be needed later when this gets implemented anyway. Or are there any other benefits other than being more nicer looking atm?

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! If the simplex tests should pass, what is missing in this PR? The general case can be done in a follow-up PR!?

Comment on lines +2186 to +2185
const unsigned int ee =
ExtractFaceHelper::reorder_face_derivative_indices<
dim>(interior_face_no, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just see this is the MappingQ path. Simplex meshes and mixed meshes should not enter this path; so you don't need to pass in the extra argument. Sorry for the confusion.

Comment on lines 3102 to 3104
if (dim == 3)
evaluate_in_face_apply<1, EvalTangent, EvalNormal>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the if-statement for dim==3 and not below for the direction 2 case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order here shouldn't actually matter because I do all the offsets in evaluate_in_face_apply. However this makes more sense since I now always put the evaluation of the normal faces last, i.e face_direction == component . It also made it easier for me to write everything up on paper beforehand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what is face_direction==2 in the 2D case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the different calls are not with different face_direction but rather the normal_dir that I pass to the tensor_evaluation_kernels. Since I'm using the "general" kernels for the normal faces, this is not used for anything other than the initial offsets of the pointers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me reformulate the question: but what is normal_dir==2 in the 2D case ;)

Copy link
Member

@peterrum peterrum May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about swapping these?

      const unsigned int face_direction = face_no / 2;

      evaluate_in_face_apply<0, EvalGeneral, EvalGeneral>(
        values_dofs,
        fe_eval,
        scratch_data,
        evaluation_flag,
        face_direction,
        subface_index,
        std::integral_constant<bool, integrate>()); // normal

      evaluate_in_face_apply<1, EvalNormal, EvalTangent>(
        values_dofs,
        fe_eval,
        scratch_data,
        evaluation_flag,
        face_direction,
        subface_index,
        std::integral_constant<bool, integrate>()); // tangential 0

      if (dim == 3)
        evaluate_in_face_apply<2, EvalTangent, EvalNormal>(
          values_dofs,
          fe_eval,
          scratch_data,
          evaluation_flag,
          face_direction,
          subface_index,
          std::integral_constant<bool, integrate>()); // tangential 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here

case 3:
eval0.template values<0, true, false, normal_dir>(values_dofs,
values_quad);
eval1.template values<1, true, false, normal_dir>(values_quad,
values_quad);
break;
case 2:
eval0.template values<0, true, false, normal_dir>(values_dofs,
values_quad);

normal_dir can be 2 although the dimension of EvaluatorTensorProduct is only 1 or 2...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you have introduced normal_dir in EvaluatorTensorProduct<evaluate_general> during the commit NiklasWik@6918f72#diff-bd08275179f5b90beff71f4688219f973ca2cb77a58e1df1cb809aff9274fce1. I have overlooked that. Could you revert that change? There is no normal-direction in the general case. I guess the only reason for that was to be able to pass normal_dir to the RT type of EvaluatorTensorProduct. This does not feel right, since we might need in the future also something else than the normal. I would suggest to encode the information of the normal in the template arguments of the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about swapping these?

I prefer this order since this is the order of the faces in the quadrature points. I think that changing it only will introduce more confusion. Unless it's beneficial from a performance pov, which I doubt..? I can add a comment explaining the order if you want.

Could you revert that change?

This will require a lot of changes... And I'm not immediately sure how I would make this pretty. I would rather skip this for now, and rather address this if I have time over at the end of my thesis. But if you really think this is necessary now then I'll of course do it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normal_dir can be 2 although the dimension of EvaluatorTensorProduct is only 1 or 2...

Maybe my explanation above wasn't that good :')

Comment on lines 3161 to 3163
const unsigned int component_table[3][3] = {{1, 2, 0},
{2, 0, 1},
{0, 1, 2}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use ndarray? Also I think this could be static.

for (unsigned int comp = 0; comp < n_components; ++comp)
value_out[comp] = this->values_quad[comp * nqp + q_point] *
jac[comp][comp] *
inv_det; // / this->jacobian[0][comp][comp];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. I was curious about whether or not it would be faster to use division here than to compute the determinant. @kronbichler thought it would be faster with the determinant (and I of course trust him on this) but I haven't measured it.

Comment on lines +5770 to +5776
// Derivatives are reordered for faces. Need to take this into account
const VectorizedArrayType inv_det =
(is_face && dim == 2 && this->get_face_no() < 2) ?
-determinant(inv_t_jac) :
determinant(inv_t_jac);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 3D not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! The determinant is the same for all cases in 3D :)

{
const Tensor<2, dim, dealii::VectorizedArray<Number>> jac =
this->jacobian[1];
const VectorizedArrayType inv_det = determinant(this->jacobian[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be simply the product of the values on the diagonal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, sorry for the confusion. The division was instead of previously computing the inverse of this->jacobian[0]. But yes, maybe the product of the diagonal elements is faster than computing the determinant. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, the product of the diagonals is faster (2 multiplications in 3D) compared to the determinant, which does 4 multiplications, 5 FMAs in 3D according to

// hard-coded for efficiency reasons
const Number C0 = internal::NumberType<Number>::value(t[1][1] * t[2][2]) -
internal::NumberType<Number>::value(t[1][2] * t[2][1]);
const Number C1 = internal::NumberType<Number>::value(t[1][2] * t[2][0]) -
internal::NumberType<Number>::value(t[1][0] * t[2][2]);
const Number C2 = internal::NumberType<Number>::value(t[1][0] * t[2][1]) -
internal::NumberType<Number>::value(t[1][1] * t[2][0]);
return t[0][0] * C0 + t[0][1] * C1 + t[0][2] * C2;

determinant(inv_t_jac);

VectorizedArrayType tmp;
// J * grad_quad * J^-1 * det(J^-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add the equation to the Cartesian case?

@@ -5945,6 +6373,12 @@ FEEvaluationAccess<dim, dim, Number, is_face, VectorizedArrayType>::
const SymmetricTensor<2, dim, VectorizedArrayType> sym_grad,
const unsigned int q_point)
{
// TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO

const std::size_t nqp = this->n_quadrature_points;
Tensor<1, dim, VectorizedArrayType> value_out;

// Cartesian cell
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would prefer if you'd move the comments inside the if-statement body.

Comment on lines 738 to 744
* A pointer to the inverse transpose Jacobian information of the present
* cell. Only set to a useful value if on a non-Cartesian cell. If the cell is
* Cartesian/affine then the transpose Jacobian is stored at index 1. For
* faces, the derivatives are reorder s.t the normal derivative is stored
* last, i.e for face_no = 0 or 1, the derivatives are ordered as
* [dy, dz, dx], face_no = 2 or 3: [dz, dx, dy], and face_no = 5 or 6:
* [dx, dy, dz].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the documentation of MappingInfo have this information? If no could you add it there as well. Could you reference here the class/field. The documentation might get out of sync at some time.

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go from my side once you have addressed https://github.com/dealii/dealii/pull/13642/files#r862829918 and solved my confusion!

@NiklasWik
Copy link
Contributor Author

I'm pretty sure that it is the matrix assembly that is taking so long for the tests (on the topic of why we can't run the testcase with dim = degree = 3). Noticed this as I'm comparing the performance, and where the actual matrix-vector product takes a fraction of the total runtime. Still of course slow compared to the mf version.

@NiklasWik
Copy link
Contributor Author

NiklasWik commented May 9, 2022

I moved the template parameter, but there are some changes in evaluation_kernels.h that I don't like, for instance in interpolate_generic_raviart_thomas_apply_face(..). There's probably a better way to do this.

Also added a little comment with a table regarding the ordering of the faces, and made the small changes from #13671 for hanging nodes support. There's ofc still the problem with the tests...

Updated tests, reordering of components in face evaluation, and storing jacobian for affine face evaluation
Update comments and documentation
+ Bug fix for general submit_values
@NiklasWik NiklasWik force-pushed the piola_matrix_free branch 2 times, most recently from 750c2f2 to c529a0e Compare May 12, 2022 12:18
Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for introducing EvaluatorTensorProductAnisotropic! I like it like this more! I guess by now you know me good enough that I would have preferred this change in a separate PR, since this change modifies a lot of unrelated places ;)

@kronbichler Could you take another look at the PR and after that let's merge!

include/deal.II/matrix_free/evaluation_kernels.h Outdated Show resolved Hide resolved
Comment on lines +3056 to +3059
// -----------------------------------------------------------------------------------
// | | Anisotropic faces | Isotropic faces|
// | Face dir | comp, coords, normal_dir | comp, coords, normal_dir | comp, coords |
// | --------------------------------------------------------------------------------|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a table! I am still not 100% happy with the code: it does not match the table. 2D does not contain any 2s; you remove the 2 division. I would suggest to get it on master, as it is and we clean up it later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left out normal_dir for the isotropic faces in the table since the parameter is not used as normal_dir for those cases, but only for shifting the pointers. I'm not sure how you would like it to be instead.

Creating new EvaluationTensorProductAnisotropic
Comment on lines 6220 to 6250
if (this->data->element_type ==
internal::MatrixFreeFunctions::ElementType::tensor_raviart_thomas)
{
// Piola transform is required

# ifdef DEBUG
Assert(this->is_reinitialized, ExcNotInitialized());
# endif
AssertIndexRange(q_point, this->n_quadrature_points);
Assert(this->J_value != nullptr,
internal::ExcMatrixFreeAccessToUninitializedMappingField(
"update_gradients"));
Assert(this->jacobian != nullptr,
internal::ExcMatrixFreeAccessToUninitializedMappingField(
"update_gradients"));
# ifdef DEBUG
this->gradients_quad_submitted = true;
# endif

const std::size_t nqp = this->n_quadrature_points;
if (!is_face &&
this->cell_type == internal::MatrixFreeFunctions::cartesian)
{
// Cartesian cell
const Tensor<2, dim, VectorizedArrayType> &inv_t_jac =
this->jacobian[0];
const Tensor<2, dim, VectorizedArrayType> &jac = this->jacobian[1];
const VectorizedArrayType weight = this->quadrature_weights[q_point];
for (unsigned int d = 0; d < dim; ++d)
for (unsigned int comp = 0; comp < n_components; ++comp)
this->gradients_quad[(comp * dim + d) * nqp + q_point] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copy-pasting this code, can we simply convert the Tensor<1, dim, Tensor<1, dim, VA>> into a Tensor<2, dim, VA>? Not sure if there is an automatic conversion, but otherwise you can simply copy the tensor and insert the code. It would be pity to repeat all the code here.

* A pointer to the Jacobian information of the present cell. Only set to a
* useful value if on a non-Cartesian cell.
* A pointer to the inverse transpose Jacobian information of the present
* cell. Only set to a useful value if on a non-Cartesian cell. If the cell is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by Only set to a useful value if on a non-Cartesian cell.? Do you mean that the index only goes by the quadrature points q for the non-affine case, while for affine cells we always need to index with [0]? And that the affine/Cartesian case also has the actual Jacobian in the same location?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was something that was already there and frankly something I too was confused about, but something I left untouched. I'll clearify it with that for affine/Cartesian cells only index 0 is set.

* A pointer to the inverse transpose Jacobian information of the present
* cell. Only set to a useful value if on a non-Cartesian cell. If the cell is
* Cartesian/affine then the Jacobian is stored at index 1. For faces on
* hypercube elements, the derivatives are reorder s.t the derivative
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit-pick:

Suggested change
* hypercube elements, the derivatives are reorder s.t the derivative
* hypercube elements, the derivatives are reorder s.t. the derivative

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great work, it is clear that you put a lot of patience and effort into this work. I like the direction of this very much, we are almost there for the RT case.

Fix documentation
@kronbichler kronbichler merged commit fac6d58 into dealii:master May 23, 2022
mkghadban pushed a commit to OpenFCST/dealii that referenced this pull request Sep 8, 2022
Matrix-free Piola transformation for affine cells
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants