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
Add get_embedding_dofs() to FE_Nedelec (2-D) #13158
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.
Code looks great! I have just some minor suggestions, mainly turning variables into const
.
06a401d
to
052acb4
Compare
Thank you, @marcfehling! I added each of your suggestions above and squashed the commits. Please let me know if you'd like to see other changes before merging. |
/rebuild |
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 don't think this function should be in the base class. But that shouldn't mean that it can't be in the Nedelec class itself, of course.
include/deal.II/fe/fe.h
Outdated
* therefore, may not implement it. | ||
*/ | ||
virtual std::vector<unsigned int> | ||
get_embedding_dofs(const unsigned int sub_degree) const; |
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 don't think this should be part of the base class. You are assuming that there is a 1:1 relationship between the dofs of a higher and lower degree element, but that is really only the case for very special cases. The more general solution for what you want to do is the get_interpolation_matrix()
function, for which your special case leads to a situation where the matrix has specific entry values.
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.
Yes, since the one to one identification is only possible in special cases I agree that it makes more sense to leave this out of the base class.
I'll take a look at implementing get_interpolation_matrix()
for a better generalization here.
include/deal.II/fe/fe_nedelec.h
Outdated
* opposed to the Nedelec degree. | ||
*/ | ||
virtual std::vector<unsigned int> | ||
get_embedding_dofs(const unsigned int sub_degree) const override; |
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.
Even for the Nedelec element, isn't this only applicable for, say, element orders 1 and n, 2 and 2n, 3 and 3n (assuming we are using equispaced points), etc?
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.
Perhaps I have misunderstood portions of the Nedelec element implementation in deal.II, but if this were the case I would have expected that this would have been indicated by the test I wrote. I also tested beforehand with the original way I was mapping from p-reduced Nedelec cells to enriched ones using the curl-conforming projection-based interpolation.
I'll have to dive back into the FE_Nedelec
implementation and see if there is something I missed that might prevent identification of equivalent dofs between a Nedelec cell of sub_degree
and a Nedelec cell of sup_degree
in this manner for some cases.
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.
Think about the usual Q_k elements in 2d. If you have a Q_2, it has DoFs at the vertices, edge midpoints, and cell midpoint. If you have a Q_3, it has DoFs at the vertices but also at positions 1/3 and 2/3 along the edge, and 4 in the interior of the cell which are all not located at the cell center. I don't think the Nedelec element has a structure that is different from the general problem outlined above unless it is moment based along edges in which all edge and cell degrees of freedom might be located at edge and cell midpoints.
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.
The case for FE_Nedelec
is not quite the same as the standard Q_k element. The support points are the Gauss-Legendre quadrature points (on the faces or the interior of the cell), but the identification of equivalent DoFs from a Nedelec element of sub_degree
to an element of sup_degree
is based on the hierarchical construction of the mixed order Nedelec polynomials as in PolynomialsNedelec<dim>::create_polynomials()
. In other words, the enriched cell includes all the shape functions of the reduced cell, so there is no concern w.r.t. the choice of sub_degree
or sup_degree
.
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 don't think this function should be in the base class. But that shouldn't mean that it can't be in the Nedelec class itself, of course.
052acb4
to
5d5339f
Compare
I removed the function from the base class for the reasons stated above and squashed the commits. Somewhat unrelated to this specific commit but related to the identification of embedding DoFs, would it be of interest to add illustrations of the shape functions (by component) on the reference cell for |
More documentation is always better! It's more difficult for vector-valued elements, of course, but could probably also be scripted. I wished I still knew where the script is (or whether it was preserved at all) with which we generated the existing figures. I know that it used a mesh with a single cell, a solution vector with exactly one one in it, and But if you wanted to go down that route, let's do it in a separate PR. |
Sounds good. I'll have to play around with the visualization for the script I wrote last week to plot these shape functions and keep things consistent with the other elements. I'll do that for a separate PR. |
If it helps, I placed a small program in #8634 (comment) that outputs all shape functions in the VTU format. It works for the Nedelec elements too. |
Thank you. The script that I wrote is similar, though I was plotting the components individually as scalar quantities. I will experiment with the visualization and submit a separate pull request soon. Otherwise, this PR should be good to go, unless there are other changes anyone would like me to make. |
include/deal.II/fe/fe_nedelec.h
Outdated
@@ -324,6 +324,18 @@ class FE_Nedelec : public FE_PolyTensor<dim> | |||
virtual std::unique_ptr<FiniteElement<dim, dim>> | |||
clone() const override; | |||
|
|||
/** | |||
* For a finite element of degree @p sub_degree < @p degree, we return a |
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.
* For a finite element of degree @p sub_degree < @p degree, we return a | |
* For a finite element of degree larger than @p sub_degree, we return a |
I think this is easier to read.
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 agree that this is easier to understand. I pushed this change.
Would you like me to make the same modification to the description for the equivalent function in FE_Q_Hierarchical
?
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.
Would you like me to make the same modification to the description for the equivalent function in
FE_Q_Hierarchical
?
I think that this would be a nice improvement, so go for it! I suggest opening a separate PR for this change.
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.
Sounds good. Will do.
5d5339f
to
fb1286c
Compare
Is this PR ready to be merged? |
Yes, I believe so. |
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 we somehow forgot to merge this one. @tamiko do we want to still merge it for the upcoming release, as the milestone suggests? I would be in favor, but we need a second opinion at this stage.
We need in particular @bangerth to OK the pull request :-) |
The change request (refactoring into the Nedelec class itself has been addressed).
My apologies for dropping the ball in the first place (and then being on vacation until Monday and in the air Tuesday+Wednesday). Thanks everyone for handling the patch, and to @harmonj for writing it in the first place! |
Add get_embedding_dofs() to FE_Nedelec (2-D)
Adds
get_embedding_dofs()
to theFE_Nedelec
element (currently fordim==2
).