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 support for hanging nodes to the NedelecSZ class. #16304
Add support for hanging nodes to the NedelecSZ class. #16304
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.
Just a more high-level review. We absolutely need tests to make sure all the nitty-gritty details are correct. 🙂
In general, it would be good to avoid more of the magic numbers.
@masterleinad, thank you so much for catching all the corrections! The code was developed incrementally over a long period, allowing several spelling mistakes to slip through. I reduced the number of magic numbers by a fair amount. That should improve the readability of the code. Also, I have developed some ideas for tests. I will add them as soon as I find the time to implement them. |
I added a first test. The test is already very comprehensive and should cover most parts of the hanging node implementation. It is a convergence test where we solve a real-valued Maxwell equation on a cylinder (or a disk in 2D). Moreover, we choose the boundary values such that we know the exact solution so that we can compare the numerical result with the exact solution. The grid and the refinement are designed to cover most configurations, but we need to do 3 iteration steps. We could reduce it into two steps, but then one special case would not be covered. I am considering turning that special case into its own test case, anyhow. This is already a very high-level test and, therefore, should catch many problems. The drawback is that the test has quite a high run time (just below 2 minutes on my system). |
@kinnewig such an important contribution for EM-focused applications, thanks for working on that! Your test is quite elaborate indeed. Additionally, you may want to also elaborate (e.g. add local refinements) tests which verify basic vector identities (especially given your fix for the hessians...), check these: https://github.com/dealii/dealii/blob/master/tests/fe/fe_nedelec_sz_gradient_divergence_theorem.cc I assume you've looked at that, but in case: have you also tested a scenario with handing nodes at non-rectangular boundaries (say boundary of a hyperball with the manifold attached) together with inhomogeneous BCs for p >= 1? |
80c2432
to
e7c3405
Compare
I turned all remarks from @agrayver into test cases. With that, this is covered comprehensively with test cases. I would be happy if one of the core developers could review the code (maybe @kronbichler or @tamiko?). At the moment, there is still one TODO left open. Currently, the implementation assumes four (or fewer) cells are adjacent to each line (in 3D). But I will fix that in a separate pull request. |
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. This was a massive amount of work to get right, so 👍 for that! I looked into the function that computes the HN constraints and it does a lot of repetitive things, both compared to the making_oldstyle_hanging_node_constraints
and for the various cases within the new code, but then there are several differences at some places, so I would not be able to point out another variant that could re-use some of this code. Thus, I am fine with getting this into the library in the present form, apart from the smaller adjustments outlined below. I will have another look once you've addressed them.
source/dofs/dof_tools_constraints.cc
Outdated
// loop over all lines; only on lines there can be constraints. We do so | ||
// by looping over all active cells and checking whether any of the faces | ||
// are refined which can only be from the neighboring cell because this | ||
// one is active. In that case, the face is subject to constraints | ||
// | ||
// note that even though we may visit a face twice if the neighboring | ||
// cells are equally refined, we can only visit each face with hanging | ||
// nodes once |
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 quite similar to the make_oldstyle_hanging_node_constraints
, right? I would suggest to reduce the comments and just put a general comment at the top that explains that we're largely following that other function, and only comment on the parts that are different.
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, the functions are based on make_oldstyle_hanging_node_constraints
.
I added the following remark (both to the 2D and 3D case):
Parts of this function are very similar to
make_oldstyle_hanging_node_constraints.
Therefore, only the parts that differ from the
make_oldstyle_hanging_node_constraints are commented on.
And I removed (or at least shortened) the comments that are copied from make_oldstyle_hanging_node_constraints
@@ -0,0 +1,448 @@ | |||
// --------------------------------------------------------------------- | |||
// | |||
// Copyright (C) 1998 - 2020 by the deal.II authors |
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.
// Copyright (C) 1998 - 2020 by the deal.II authors | |
// Copyright (C) 2023 - 2024 by the deal.II authors |
(Here and elsewhere.)
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 adapted those lines in all tests.
Should I update the copyright years also in the files that are already present (include/deal.II/fe/fe_nedelec_sz.h
, source/dofs/dof_tools_constraints.cc
and source/fe/fe_nedelec_sz.cc
)?
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 end year is not so important, because we have scripts doing it automatically. We try to be reasonably precise for the first year a file was created (which is why I made the comment), so feel free to make. But it is not super important, so either way is fine.
…SZ elements are handled correctly.
…h hanging nodes work as expected.
e7c3405
to
f16f94d
Compare
btw, I forgot there is the |
Let us merge this PR now; I think this is a great progress, possible follow-up topics can be addressed later. |
Back on the deal.II user workshop in Hanover, I promised to create a pull request to add the hanging node support for FE_NedelecSZ elements soon. However, I was busy working on the Tpetra wrappers instead, as I needed the Tpetra interface for another project. But now I finally found the time to clean up the code and make some final changes.
I am happy to create this pull request, introducing support for hanging nodes to the FE_NedelecSZ elements!
The theory behind the implementation is covered in my paper: Algorithmic realization of the solution to the sign conflict problem for hanging nodes on hp-hexahedral Nédélec elements.
I have not added any tests to this pull request jet since the tests presented in the paper take too long to evaluate (especially in 3D since we need a polynomial degree of p = 2 to cover every case). So, if you have any suggestions for more performant tests, please let me know, and I will be happy to include them!