Provide more general factory class for FEEvaluation #10927

kronbichler · 2020-09-17T14:27:13Z

This PR adds a factory class for the evaluate and integrate calls of FEEvaluation and FEFaceEvaluation. As opposed to SelectEvaluator which seems to rebuild the code every time the class is used, this places the code in a separate file to force separation. It could have been achieved with that class also with some work but I did not want to repeat everything for FEFaceEvaluation - I have chosen a shorter approach with less code.

Right now I use a FE_EVAL_FACTORY_DEGREE_MAX to let the user pre-compile different degrees. We could think of making that a cmake variable to more easily change things. However, the question that is still open is the range for the quadrature formulas, which is hardcoded right now.

Whenever the code hits the pre-compiled part but with a degree not expanded, we fall back to the generic evaluate function that does not have the template (and is >2x slower).

Fixes #9794. But I must admit that we currently get a bigger library with this PR because code gets pre-compiled for all types of VectorizedArray.

In a follow-up PR, we should clean up the internal interfaces because it is ugly to pass around that many arguments, especially in the gather_evaluate and integrate_scatter calls.

include/deal.II/matrix_free/evaluation_template_factory.templates.h

kronbichler · 2020-09-17T14:40:18Z

source/matrix_free/evaluation_template_factory.inst.in

+
+
+for (deal_II_dimension : DIMENSIONS;
+     deal_II_scalar_vectorized : REAL_SCALARS_VECTORIZED)


This is the reason why it gets expensive - we need to compile it for all VectorizedArray types we have. We could again think of a shortcut to only use fast paths for some degrees, but I fear that we would need to be careful with describing this for the user to not give surprises when performance differs by a factor of 2.

I am fine with your suggestion. I guess I am one of the few who actually uses the VectorizedArrayType template argument.

peterrum

Looks good to me once the tests are happy.

In a follow-up PR, we should clean up the internal interfaces because it is ugly to pass around that many arguments, especially in the gather_evaluate and integrate_scatter calls.

Indeed, we need to do something here!

peterrum · 2020-09-17T18:52:30Z

include/deal.II/matrix_free/fe_evaluation.h

+  if (fe_degree > -1)
+    SelectEvaluator<dim, fe_degree, n_q_points_1d, VectorizedArrayType>::
+      evaluate(n_components,
+               evaluation_flags,
+               *this->data,
+               const_cast<VectorizedArrayType *>(values_array),
+               this->values_quad,
+               this->gradients_quad,
+               this->hessians_quad,
+               this->scratch_data);
+  else
+    internal::FEEvaluationFactory<dim, Number, VectorizedArrayType>::evaluate(
+      n_components,
+      evaluation_flags,
+      *this->data,
+      const_cast<VectorizedArrayType *>(values_array),
+      this->values_quad,
+      this->gradients_quad,
+      this->hessians_quad,
+      this->scratch_data);


This is not really nice. Duplication of the same arguments. The same in the following three functions.

I agree, but let us solve this with #10928 because that would reduce the duplication to 3 (?) arguments, at which point duplication is very likely the more readable version compared to adding a layer to collect the two directions (e.g. lambda).

peterrum · 2020-09-17T18:55:26Z

source/matrix_free/evaluation_template_factory.inst.in

+
+
+for (deal_II_dimension : DIMENSIONS;
+     deal_II_scalar_vectorized : REAL_SCALARS_VECTORIZED)


I am fine with your suggestion. I guess I am one of the few who actually uses the VectorizedArrayType template argument.

peterrum · 2020-09-17T18:56:05Z

source/matrix_free/evaluation_template_factory.inst.in

+    template bool dealii::internal::FEFaceEvaluationFactory<
+      deal_II_dimension,
+      deal_II_scalar_vectorized::value_type,
+      deal_II_scalar_vectorized>::
+      gather_evaluate<1>(
+        const unsigned int,
+        const deal_II_scalar_vectorized::value_type *,
+        const MatrixFreeFunctions::ShapeInfo<deal_II_scalar_vectorized> &,
+        const MatrixFreeFunctions::DoFInfo &,
+        deal_II_scalar_vectorized *,
+        deal_II_scalar_vectorized *,
+        deal_II_scalar_vectorized *,
+        const bool,
+        const bool,
+        const unsigned int,
+        const unsigned int,
+        const std::array<unsigned int, 1>,
+        const std::array<unsigned int, 1>,
+        const unsigned int,
+        const MatrixFreeFunctions::DoFInfo::DoFAccessIndex,
+        const std::array<unsigned int, 1>,
+        const Table<2, unsigned int> &);
+
+    template bool dealii::internal::FEFaceEvaluationFactory<
+      deal_II_dimension,
+      deal_II_scalar_vectorized::value_type,
+      deal_II_scalar_vectorized>::
+      integrate_scatter<1>(
+        const unsigned int,
+        deal_II_scalar_vectorized::value_type *,
+        const MatrixFreeFunctions::ShapeInfo<deal_II_scalar_vectorized> &,
+        const MatrixFreeFunctions::DoFInfo &,
+        deal_II_scalar_vectorized *,
+        deal_II_scalar_vectorized *,
+        deal_II_scalar_vectorized *,
+        deal_II_scalar_vectorized *,
+        const bool,
+        const bool,
+        const unsigned int,
+        const unsigned int,
+        const std::array<unsigned int, 1>,
+        const std::array<unsigned int, 1>,
+        const unsigned int,
+        const MatrixFreeFunctions::DoFInfo::DoFAccessIndex,
+        const std::array<unsigned int, 1>,
+        const Table<2, unsigned int> &);


This file is ugly^^

peterrum · 2020-09-17T20:15:21Z

By the way, I think this is a change worth a change-log entry, isn't it?

kronbichler · 2020-09-21T07:59:26Z

I added two new tests for the features that are new, and switched some existing test cases to use to -1 template path. Should be ready from my point of view.

peterrum

Looks good. I would probably merge tomorrow if there are no objections.

tjhei · 2020-09-21T13:17:14Z

I don't understand the internal changes of this PR, but I feel that it would be great to have a paragraph about this feature in the matrix-free documentation somewhere (especially how FE_EVAL_FACTORY_DEGREE_MAX works).

Is there a way for the user to know if the current setup is in the "slow path"?

kronbichler · 2020-09-22T09:53:46Z

I don't understand the internal changes of this PR, but I feel that it would be great to have a paragraph about this feature in the matrix-free documentation somewhere (especially how FE_EVAL_FACTORY_DEGREE_MAX works).

Very good point. I added documentation for the parameter in FEEvaluation.

Is there a way for the user to know if the current setup is in the "slow path"?

I guess we should provide a library function which prints the information to a stream given by the user. Let me try that in a follow-up PR - I open an issue for now to recall this and possibly extend the documentation.

peterrum · 2020-09-22T10:54:04Z

include/deal.II/matrix_free/fe_evaluation.h

+ * templates), deal.II also supports usage of this class based on the
+ * information in the element passed to the initialization. For this usage
+ * model, set the template parameter for the polynomial degree to -1 and
+ * choose an arbitrary number for the number of quadrature points. That code


arbitrary number for the number of quadrature points

The value is not actually used, is it? I guess we cannot force it to be -1 due to backwards compatibility.

No, it is not used, but also it does not matter I believe because we replace the value here:

dealii/include/deal.II/matrix_free/evaluation_template_factory.templates.h

Line 55 in 1b5806b

return EvaluatorType::template run<-1, 0>(args...);

If I remember exactly we also did not say explicitly that one needed the combination -1,0 before, so I would think it does not matter. Or was there anything particular you had in mind that works in one case but not the other?

Thanks. No, I think it might give somehow to a user the wrong impression if n_quadrature_points is not forced to -1 and he/she sets the values to >0 and gets the wrong (not expected) result.

But this is not that important for this PR. I am happy with the PR. I let @tjhei merge!

tjhei · 2020-09-22T15:48:25Z

I guess we should provide a library function which prints the information to a stream given by the user. Let me try that in a follow-up PR - I open an issue for now to recall this and possibly extend the documentation.

Personally, I would want this to be on by default on anything I work on: if I use matrix-free, I care about performance. A warning message when starting is not something that would bother me. I guess we don't have this in any other place in the code, though.

peterrum · 2020-09-22T19:12:14Z

@kronbichler I guess I am speaking on behalf of every MatrixFree user who has to deal with many FEEvaluation instances with different degrees and different number of quadrature points: this is a great addition (and not a minor one - although no one actually sees it)! Thank you for the lot of work of the last weeks to prepare this PR.

kronbichler added Matrix-free ready to test labels Sep 17, 2020

kronbichler commented Sep 17, 2020

View reviewed changes

include/deal.II/matrix_free/evaluation_template_factory.templates.h Outdated Show resolved Hide resolved

kronbichler force-pushed the fix_9794 branch from c400519 to 42cac17 Compare September 17, 2020 14:33

peterrum self-requested a review September 17, 2020 14:36

kronbichler mentioned this pull request Sep 17, 2020

Interfaces to FEEvaluationImpl & co #10928

Closed

kronbichler commented Sep 17, 2020

View reviewed changes

peterrum reviewed Sep 17, 2020

View reviewed changes

peterrum mentioned this pull request Sep 18, 2020

Add some functions to FEEvaluation #10931

Merged

kronbichler added 4 commits September 18, 2020 14:44

Implement factory class for FEEvaluation and FEFaceEvaluation

dc4efca

Use new factory class within library

42e7433

Add 3/2 over-integration to list of instantiated quadratures

5e67295

Add inverse mass matrix with degree -1

d4697e2

kronbichler force-pushed the fix_9794 branch from 42cac17 to d4697e2 Compare September 18, 2020 15:36

peterrum approved these changes Sep 18, 2020

View reviewed changes

kronbichler added 2 commits September 21, 2020 09:55

Convert a few tests to template parameter -1

67101d5

New test cases for CellwiseInverseMassMatrix<dim,-1>

d96e09d

peterrum approved these changes Sep 21, 2020

View reviewed changes

kronbichler added 2 commits September 21, 2020 10:41

Remove obsolete code

4f7d0ca

Changelog

e6d7131

kronbichler force-pushed the fix_9794 branch from 73cd11c to e6d7131 Compare September 21, 2020 11:30

peterrum mentioned this pull request Sep 22, 2020

Remove n_face_orientations template argument within FEEvaluation #10946

Merged

Add documentation

1b5806b

kronbichler mentioned this pull request Sep 22, 2020

Document pre-compiled polynomial degrees with FEEvaluation #10947

Closed

peterrum reviewed Sep 22, 2020

View reviewed changes

peterrum approved these changes Sep 22, 2020

View reviewed changes

tjhei merged commit bed997f into dealii:master Sep 22, 2020

kronbichler mentioned this pull request Sep 25, 2020

Remove unnecessary instantiation files #10970

Merged

kronbichler deleted the fix_9794 branch October 1, 2020 11:21

kronbichler mentioned this pull request Mar 12, 2023

Implement InverseCellwiseMassMatrix with tensor var. coeff. #14850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide more general factory class for FEEvaluation #10927

Provide more general factory class for FEEvaluation #10927

kronbichler commented Sep 17, 2020 •

edited

kronbichler Sep 17, 2020

peterrum Sep 17, 2020

peterrum left a comment

peterrum Sep 17, 2020

kronbichler Sep 18, 2020

peterrum Sep 18, 2020

peterrum Sep 17, 2020

peterrum Sep 17, 2020

peterrum commented Sep 17, 2020

kronbichler commented Sep 21, 2020

peterrum left a comment

tjhei commented Sep 21, 2020

kronbichler commented Sep 22, 2020

peterrum Sep 22, 2020

kronbichler Sep 22, 2020

peterrum Sep 22, 2020

tjhei commented Sep 22, 2020

peterrum commented Sep 22, 2020



		for (deal_II_dimension : DIMENSIONS;
		deal_II_scalar_vectorized : REAL_SCALARS_VECTORIZED)

Provide more general factory class for FEEvaluation #10927

Provide more general factory class for FEEvaluation #10927

Conversation

kronbichler commented Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrum commented Sep 17, 2020

kronbichler commented Sep 21, 2020

peterrum left a comment

Choose a reason for hiding this comment

tjhei commented Sep 21, 2020

kronbichler commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjhei commented Sep 22, 2020

peterrum commented Sep 22, 2020

kronbichler commented Sep 17, 2020 •

edited