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
FECouplingValues. #15773
FECouplingValues. #15773
Conversation
@tjhei , would you mind giving a look at the snippet of code above and share your thoughts about the interface? Before I move forward and modify our FEvaluesViews classes, I'd like to have a consensus on how the interface should look like. |
cacb25c
to
419d701
Compare
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 think this looks very reasonable. Is there a way to avoid the overhead of always having the permutation arrays present? Would it make sense to treat the identity differently. I expect this would cost performance otherwise.
I'm not sure if this
is faster than unconditionally doing
also for the identity. Opinions? I don't have a preference. |
So these are the steps needed for the concept presented here to work:
|
I would think a single
Either option is fine with me. What do you prefer? |
521c52e
to
4dfa2e1
Compare
Depends on #15819. Only the second commit introduces the actual FECouplingValues class. |
e0355a8
to
c77b733
Compare
This is now fully functional.
|
4b7bad6
to
976d413
Compare
The test dealii/include/deal.II/fe/fe_coupling_values.h Lines 1515 to 1516 in 976d413
How would you loop (within this interface) over DoF indices if |
I added the assert later, and it was in the wrong direction. Now it should work. |
6ff5f00
to
5445126
Compare
5445126
to
900e1ec
Compare
900e1ec
to
e7ba9a1
Compare
e7ba9a1
to
2705e86
Compare
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.
Looks good to me. Some minor comments. My biggest issue is that this might only work in serial (I am not counting p:s:T as parallel). The surface mesh and the bulk mesh are normally independent meshes, which are independently partitioned.
/** | ||
* Helper struct to associate an extractor to the left FEValuesBase object. | ||
*/ | ||
template <typename Extractor> | ||
struct LeftCoupling | ||
{ | ||
LeftCoupling(const Extractor &extractor) | ||
: extractor(extractor) | ||
{} | ||
|
||
/** | ||
* The actual extractor object. | ||
*/ | ||
const Extractor extractor; | ||
}; | ||
|
||
/** | ||
* Helper struct to associate an extractor to the right FEValuesBase object. | ||
*/ | ||
template <typename Extractor> | ||
struct RightCoupling | ||
{ | ||
RightCoupling(const Extractor &extractor) | ||
: extractor(extractor) | ||
{} | ||
|
||
/** | ||
* The actual extractor object. | ||
*/ | ||
const Extractor extractor; | ||
}; |
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.
These two classes look identical!? What is the reason?
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.
They are used to identify which of the two FEValuesBase
you are addressing. Asking for fev[left]
and fev[right]
will have a different effect.
This actually works perfectly fine on p::d::T (bulk) coupled with p::s::T (surface). Moreover, for BEM problems, once you set up the hierarchical matrices (based on shared trees), the p::s::T can be safely discarded. And one can create a surface p::f::T where the partitioning is induced by the (bulk) p::f::T (or p::d::T). |
c815158
to
310b7a3
Compare
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 think this looks good in principle, it is an exciting feature. However, I have two main concern, one regarding the naming left
/right
discussed further down, and one regarding the overall performance aspects. While I agree that the present class is nice, it is another incarnation (besides the FEValuesViews
, FEInterfaceValues
, etc) that fundamentally makes a slow-performance choice: For usability, we decide to return zeros whenever we are on the other side regarding the index. Do not get me wrong, this is not bad, but if one ever wants to us this in a context where performance matters, it seems this moves us in the wrong direction. This PR gets my approval because I do not have the resources to think for a good solution. Since we already have FEPointEvaluation
or FEEvaluation
in the matrix-free module that essentially provide 80% of what is required to assemble the coupling matrices, plus tensor product libraries like https://github.com/hyperdeal/hyperdeal that have algorithms for tensor product coupling, I strongly suggest that, in case performance starts to become relevant, to start looking there and talk to me.
I perfectly agree with your concerns. This is not thought to be fast for the computer, but fast for the programmer, similarly to Indeed, if we want to go fast, we need to take another route. However, I think that having this (slow) approach available is also necessary if one wants to be able to have a readable way to build these problems. I think this is a possible interface that follows the same steps we took elsewhere, and it would make it very easy to write coupled problems (with the same overhead of FEValuesViews/FEValues, etc.). If you see obvious ways in which this could be made faster, let's discuss it further. :) |
Ping @dealii/dealii ? |
Would you mind rebasing this to see whether the tests still all succeed? |
989dd93
to
2dbe391
Compare
Only 68ab103 is relevant. |
2dbe391
to
68ab103
Compare
Thanks. I will look at that over the next couple of days. |
Could you rebase now that the other patch has been merged? That should make the first commit go away. |
68ab103
to
dad2cbe
Compare
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.
Part 1. The rest later today.
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.
Here's the rest. Nothing that will require much work.
I appreciate how extensively all of this is documented, and that you have 5 new tests. Good work.
/** | ||
* Helper struct to associate an extractor to the first FEValuesBase object. | ||
*/ | ||
template <typename Extractor> | ||
struct FirstCoupling | ||
{ | ||
FirstCoupling(const Extractor &extractor) | ||
: extractor(extractor) | ||
{} | ||
|
||
/** | ||
* The actual extractor object. | ||
*/ | ||
const Extractor extractor; | ||
}; | ||
|
||
/** | ||
* Helper struct to associate an extractor to the second FEValuesBase object. | ||
*/ | ||
template <typename Extractor> | ||
struct SecondCoupling | ||
{ | ||
SecondCoupling(const Extractor &extractor) | ||
: extractor(extractor) | ||
{} | ||
|
||
/** | ||
* The actual extractor object. | ||
*/ | ||
const Extractor extractor; | ||
}; |
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.
What would you think of putting these classes into a namespace FEValuesExtractors::FECouplingValues
? This way they show up in the documentation in a similar place as the other extractors.
5e77cfb
to
588ac3d
Compare
@bangerth, I should have addressed all your comments. Thanks for your review! |
Thank you! |
This is
a draft PR that I would like to use as a discussiona class that can be used for general coupling operators.I would like to compute bilinear forms of the following abstract type
where$T_1$ and $T_2$ are two arbitrary sets (cells, faces, edges, or any$K$ is a (possibly singular) coupling kernel,$K$ :
combination thereof), and
for three different types of Kernels
function of positive powers
points
of negative powers
the integral above is actually a single integral over the intersection of
the two sets
For the first case, one may think that the only natural way to proceed is to
compute the double integral by simply nesting two loops:
where$x_1^{q_1}$ and $x_2^{q_2}$ are the quadrature points in $T_1$ and
$T_2$ respectively, and $w_1^{q_1}$ and $w_2^{q_2}$ are the corresponding$T_1\times T_2$ . For singular Kernels, for example, this is often the only way to$T_1\times T_2$ is usually not
quadrature weights.
This, however is not the only way to proceed. In fact, such an integral can
be rewritten as a single loop over two vectors of points with the same length
that can be thought of as a single quadrature rule on the set
proceed, since the quadrature formula on
written as a tensor product quadrature formula, and one needs to build a
custom quadrature formula for this purpose.
This class allows one to treat the three cases above in the same way, and to
approximate the integral as follows:
Since the triple of objects$({q}, {w}, {\phi})$ is usually provided by$T_1$ and $T_2$ can be two arbitrary cells,$\phi^1_i$ and $\phi^2_j$ are basis$T_1$ and $T_2$ , respectively.$T_1$ and $T_2$
a class derived from the FEValuesBase class, this is the type that the class
needs at construction time.
faces, or edges belonging to possibly different meshes (or to meshes with
different topological dimensions),
functions defined on
The most common case of the dirac Distribution is when
correspond to the common face of two neighboring cells. In this case, this
class provides a functionality which is similar to the FEInterfaceValues
class, and provides a way to access values of basis functions on the
neighboring cells, as well as their gradients and Hessians, in a unified
fashion, on the face.
Similarly, this class can be used to couple bulk and surface meshes across
the faces of the bulk mesh. In this case, the two FEValuesBase objects will
have different topological dimension (i.e., one will be a cell in a
co-dimension one triangulation, and the other a face of a bulk grid with
co-dimension zero), and the CouplingQuadratureType argument is usually chosen
to be CouplingQuadratureType::reorder, since the quadrature points of the two
different FEValuesBase objects are not necessarily generated with the same
ordering.
The type of integral to compute is controlled by the
CouplingQuadratureType
argument (see the documentation of that enum class for more details).
An example usage of this class to assemble two coupled bilinear forms (for
example, for Boundary Element Methods) is the following:
I would like some feedback on the interface before I dive into the implementation of more involuted complex things. The minimal test shows how the remapping of quadrature points and dof indices should be.
In principle, by augmenting the constructors of the current FEValuesViews with two renumbering vectors for dofs and quads (which are ignored if of zero lenght), I think we could use the full infrasctructure of the FEValuesViews which is already in place, without duplicating code.
Thoughts?
@pcafrica, @lwy5515, @fdrmrc