Construction of FEInterfaceValues with two pointers #115
Conversation
| @@ -1546,13 +1554,13 @@ class FEInterfaceValues | |||
| const unsigned int fe_index = numbers::invalid_unsigned_int); | |||
|
|
|||
| /** | |||
| * Return a reference to the FEFaceValues or FESubfaceValues object | |||
| * Return a reference to the FEValuesBase object | |||
There was a problem hiding this comment.
Can you mention that this is typically an FEFaceValues or FESubfaceValues except in case of using the constructor that uses two FEValuesBase pointers?
| * FEValuesBase like objects. This may occur when one has | ||
| * | ||
| */ | ||
| FEInterfaceValues(FEValuesBase<dim, spacedim> *const fe_values0, |
There was a problem hiding this comment.
Why pointers instead of references?
| /** | ||
| * Construct a new FEInterfaceValues on top of two already initialized | ||
| * FEValuesBase like objects. This may occur when one has | ||
| * |
There was a problem hiding this comment.
please complete documentation
| @@ -2203,14 +2211,14 @@ class FEInterfaceValues | |||
| * Pointer to internal_fe_face_values or internal_fe_subface_values, | |||
| * respectively as determined in reinit(). | |||
| */ | |||
| FEFaceValuesBase<dim, spacedim> *fe_face_values; | |||
| FEValuesBase<dim, spacedim> *fe_face_values; | |||
There was a problem hiding this comment.
can you update the comment here as well (it can be one of three options now)
|
|
||
| /** | ||
| * Pointer to internal_fe_face_values_neighbor, | ||
| * internal_fe_subface_values_neighbor, or nullptr, respectively | ||
| * as determined in reinit(). | ||
| */ | ||
| FEFaceValuesBase<dim, spacedim> *fe_face_values_neighbor; | ||
| FEValuesBase<dim, spacedim> *fe_face_values_neighbor; |
| @@ -300,7 +300,7 @@ namespace Step12 | |||
| ScratchData<dim> & scratch_data, | |||
| CopyData & copy_data) { | |||
| scratch_data.fe_interface_values.reinit(cell, face_no); | |||
| const FEFaceValuesBase<dim> &fe_face = | |||
| const FEValuesBase<dim> &fe_face = | |||
There was a problem hiding this comment.
So this change is not backwards-compatible? Hrm.
One could also leave the old type and fail if one calls this function after using the new constructor. What do you think?
There was a problem hiding this comment.
Unfortunately, yes. The main problem is that get_fe_face_values() is called internally to compute jumps, averages, etc. so it must be available also with the new cstr. At the same time, the name get_fe_face_values() is now misleading, as in the present PR it's not returning a FEFaceValuesBase anymore, so I agree one should use the old types.
To be more specific, everything boils down to the fact that fe_face_values and fe_face_values_neighbor now need to be of type FEValuesBase.
So far the only solution I see is to add two FEValuesBase* members that will be initialized in case one chooses the new constructor. This would propagate some asserts and if sentences to the code, though.
|
In the long run, I think a better option would be (and this would also be applicable to another application we are working on with @lwy5515) to create a base class FECouplingValues, that internally stores pointers to FEvaluesBase and that does all the dof handling stuff (gathering of the indices, local to global, etc.), of which FEInterfaceValues is a derived class that knows that the FEValues passed to the base class are actually FEFaceValues, thus separating the concerns (i.e., the dof indices handling part would be done in the base class, while the jumps/averages/etc. (that require knowledge of normals/subfaces/etc.) are handled in the derived class. What you are doing here is ok, but I think we should refactor things a bit. I suggest we postpone this PR for a few days, until we discuss about this also with Martin. |
|
Sounds perfect to me! |
This PR allows to construct a
FEInterfaceValuesout of two pointers to already initialized FEValues objects. It's working for our application, but right now I'm not sure this is what you intended.First, the only context I can imagine this is when one has two
ImmersedQuadratures (and hence two differentFEImmersedSurfaceValueson two neighboring cells) and needs to compute jumps, etc. The issue with this is that a FEImmersedSurfaceValues is an FEValuesBase, not a FEFaceValuesBase and as such we don't havereinit(cell,f)), so the two internal pointers have been changed to plain FEValuesBase.Second, the
reinitof FEInterfaceValues allows also for subfaces in principle, but this is not allowed with anFEImmersedSurfaceValues, so these two Asserts for subfacesdealii/include/deal.II/fe/fe_interface_values.h
Lines 2505 to 2506 in 2190700
dealii/include/deal.II/fe/fe_interface_values.h
Lines 2521 to 2522 in 2190700
are necessary in case one accidentally calls reinit with subfaces arguments using this constructor.
@luca-heltai