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

Hessians on general cells and faces #12744

Merged
merged 1 commit into from Sep 9, 2021

Conversation

bergbauer
Copy link
Contributor

@bergbauer bergbauer commented Sep 6, 2021

This PR implements the evaluation and integration of hessians for general (non-affine) cells/faces in the matrix-free infrastructure.

Closes #10401

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.

Very nice!

doc/news/changes/minor/20210906MaximilianBergbauer Outdated Show resolved Hide resolved
include/deal.II/matrix_free/fe_evaluation.h Outdated Show resolved Hide resolved
include/deal.II/matrix_free/fe_evaluation.h Outdated Show resolved Hide resolved
include/deal.II/matrix_free/fe_evaluation.h Outdated Show resolved Hide resolved
include/deal.II/matrix_free/mapping_info.templates.h Outdated Show resolved Hide resolved
@kronbichler
Copy link
Member

/rebuild

@bergbauer
Copy link
Contributor Author

I applied your suggestions and fixed the failing tests.

@kronbichler kronbichler changed the title Hessains on general cells and faces Hessians on general cells and faces Sep 6, 2021
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.

This is really a substantial improvement so I think it is well-placed there. Thanks a lot for the effort @bergbauer !

I agree with @kronbichler here! I am looking forward to try it out in my small matrix-free biharmonic solver 🥇

Comment on lines -4346 to +4371
hessians_quad =
gradients_from_hessians_quad =
this->scratch_data_array->begin() +
n_components * (dofs_per_component + (dim + 1) * n_quadrature_points);
this->scratch_data =
this->scratch_data_array->begin() + n_components_ * dofs_per_component +
(n_components_ * ((dim * (dim + 1)) / 2 + dim + 1) * n_quadrature_points);
hessians_quad =
this->scratch_data_array->begin() +
n_components * (dofs_per_component + (2 * dim + 1) * n_quadrature_points);
this->scratch_data = this->scratch_data_array->begin() +
n_components_ * dofs_per_component +
(n_components_ * ((dim * (dim + 1)) / 2 + 2 * dim + 1) *
n_quadrature_points);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but don't we only want the memory if MF was actually set up with Hessian enabled?

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 what we did before. We maybe waste 4 or 5 instructions here to get the intermediate pointers, against an if and some bookkeeping to check if we had hessians filled in MappingInfo, so I think the code is cleaner this way. The local arrays are small enough to not really eat much into the memory of a global finite element computation, and in case we have no hessians we will never touch them (and hence not waste cache memory).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move it to the end of scratch_data_array and disable it in case the flag UpdateFlags::update_jacobian_grads is not set in mapping_data->update_flags_inner_faces. This can also be done in a follow up if we think it is necessary.

Comment on lines +6464 to +6465
const Tensor<2, dim, VectorizedArrayType> jac = this->jacobian[q_point];
const VectorizedArrayType JxW = this->J_value[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.

Suggested change
const Tensor<2, dim, VectorizedArrayType> jac = this->jacobian[q_point];
const VectorizedArrayType JxW = this->J_value[q_point];
const Tensor<2, dim, VectorizedArrayType>& jac = this->jacobian[q_point];
const VectorizedArrayType& JxW = this->J_value[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.

We are a bit inconsistent here, as we do

const Tensor<2, dim, VectorizedArrayType> &jac =
this->jacobian[this->cell_type > internal::MatrixFreeFunctions::affine ?
q_point :
0];
vs
const Tensor<2, dim, VectorizedArrayType> jac =
this->cell_type > internal::MatrixFreeFunctions::affine ?
this->jacobian[q_point] :
this->jacobian[0];

In the end, it will not matter much because a sane compiler will likely treat both cases similarly and avoid temporaries as much as possible. I am not sure if aliasing could force the compiler to emit unnecessary loads in the case of a reference and if that was the reason to do a copy in the second appearance for submit_gradient. I suggest we leave this as is (no reference) and just do something in case we have verified the reference does not lead to unnecessary loads in the multi-component case in the assembly code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could show me how to look for this in assembly code.

@@ -8281,12 +8348,19 @@ FEEvaluation<dim,
Number,
VectorizedArrayType>::
evaluate(const VectorizedArrayType * values_array,
const EvaluationFlags::EvaluationFlags evaluation_flags)
const EvaluationFlags::EvaluationFlags evaluation_flag)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we normally write flags (plural)?

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 the only place where plural was used. I changed it to singular to be consistent.

include/deal.II/matrix_free/fe_evaluation.h Outdated Show resolved Hide resolved
include/deal.II/matrix_free/mapping_info.templates.h Outdated Show resolved Hide resolved
@kronbichler kronbichler merged commit bcf1789 into dealii:master Sep 9, 2021
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.

Implement FEEvaluation::submit_hessian()
3 participants