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
[WIP] Introduce FERemoteEvaluation #15982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments. Some tests and a change-log entry would be good!
/** | ||
* Offset to data after last call of `reinit()`. | ||
*/ | ||
unsigned int data_offset = numbers::invalid_unsigned_int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should start
and cell_start
in FEREDataView
also be initialized in a constructor to conform with the deal.II
style (even though no constructor is present yet) or is it OK?
int n_components_, | ||
int dim_> | ||
void | ||
copy_data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is missing.
VectorizedArrayType>, | ||
true> | ||
{ | ||
static constexpr bool cell_face_pairs = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be true!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This is because FEFaceEvaluation::reinit(face_index)
takes a unique face batch ID instead of a cell_index
and face_number
. This is contrary to FEPointEvaluation::reinit(cell_index,face_number)
(if setup for faces).
* if @p FEEvaluationType works on faces. | ||
*/ | ||
template <typename FEEvaluationType_, bool is_face_> | ||
class FERemoteEvaluationCommunicator : public Subscriptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this class not internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FERemoteEvaluationCommunicator
is set up by the user and handed to FEPointEvaluation
. I think we should avoid that classes and functions, intended to end up in user-code, are internal
. I am open for discussion though :)
Let me summarize the purpose of the classes:
@jh66637 Is the description correct? I am wondering if |
@peterrum Your description is perfect. Filling the data without the communicator is tedious. The information on the memory positions of values and gradients are known by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the code and added some further comments. At the moment, the code is hard to read because of the many template arguments.
Filling the data without the communicator is tedious. The information on the memory positions of values and gradients are known by DataView which is stored in the Communicator.
I still think to split up the responsibilities makes sense. One class contains the data. Another one adds the notion of cells/faces (by offsets). Another fills the data by communication. And the last class accesses the data using the offsets.
Let's assume that the data is not filled via communication but contains some precomputed quantities, e.g., linerization points in a non-linear context or some variable coefficients. This can be filled quite easily manually and all classes but the one which communicates would be useful in this context.
auto rpe = | ||
std::make_shared<Utilities::MPI::RemotePointEvaluation<dim>>(tol, | ||
false, | ||
0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the rtree_level
which is defaulted to 0 if not specified.
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
Co-authored-by: Peter Munch <peterrmuench@gmail.com>
The implementations are outdated. I changed a lot during the last weeks so I will close this. All of the suggestions from the review have been included in the new implementations. |
The new class can be used to access values in quadrature points on remote triangulations similarly to FEEvaluation, FEFaceEvaluation, and FEPointEvaluation. It can be used for volume coupling as well as the coupling of non-matching interfaces.
Current implementations use quadrature points dictated by integration points of given FEEvaluation type objets. However, the structure of the classes is chosen with mortaring (with CGAL and FEPointEvaluation) in mind.
@peterrum @fdrmrc