Skip to content
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

Straight-forward tensor-product version of FE_RaviartThomasNodal #12454

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

kronbichler
Copy link
Member

@kronbichler kronbichler commented Jun 12, 2021

This PR (based on work by @kkormann and me) aims to define a simple tensor product version of FE_RaviartThomas with nodal polynomials. It avoids some of the orientation issues among the unknowns of the elements as all DoFs are node values and thus set up homogeneously among a face, but obviously it still needs the usual sign flip for some face orientations in 2D/3D. @konsim83 this is the code I was talking about: It still profits from all the work you have done, but on the other hand it can be the basis for later extensions towards efficient tensor product evaluators as described in #9545. It would be great if you can have a look to discuss if the overall concept makes sense (as I am less experienced with Piola-transformed setups).

If we find this useful, @kkormann and I have a similar version for FE_NedelecNodal around. I am not sure how many additional difficulties could arise there compared to the RT case, but since all of a face/edge has the same signs due the nodal polynomials, it might actually be a manageable task.

@agrayver FYI

Comment on lines +574 to +577
DEAL::Normal jumps (at quad points) in cell 0_0: at face 1
DEAL:: interface jumps: 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000
DEAL::Normal jumps (at quad points) in cell 1_0: at face 0
DEAL:: interface jumps: 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000 0.0000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these lines in the test indicate that the conformity tests are successful, so there should be no orientation issue.

@konsim83
Copy link
Contributor

Great work, imho, the concept makes sense and is simpler than the nodal version before. The conformity test indicates that the permutation table with signs is implemented correctly. I have the feeling that together with the tensor product Nedelec version you mentioned this can form a full de Rham complex, i.e., the tensor product Nedelec and RaviartThomas are compatible. Is this correct?

@konsim83
Copy link
Contributor

Also, would it make sense to add a conformity test for 2D since the class is dimension templated? Just to be sure that this is working.

Copy link
Member

@peterrum peterrum left a 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. Could you elaborate why some of the tests fail. I also think a incompatibility change log entry might be justified, since the location of the support points have changed.

@kronbichler
Copy link
Member Author

The tests fail because I did not yet update the output - as you say, this is an incompatible change and we change the representation of the shape functions. However, I wanted to first check with @konsim83, who is a knowledgeable person on this topic, to whether the overall direction makes sense.

Also, would it make sense to add a conformity test for 2D since the class is dimension templated? Just to be sure that this is working.

That test does exist already, https://github.com/dealii/dealii/blob/master/tests/fe/fe_conformity_dim_2_fe_rt_nodal.cc (it helped me to understand why I need the mapping_kind = mapping_raviart_thomas flag.

@kronbichler kronbichler force-pushed the update_raviart_thomas_nodal branch 2 times, most recently from 40c4f6f to b75b602 Compare July 5, 2021 16:20
@kronbichler
Copy link
Member Author

I now updated the test output. I deleted one test fe/rtdiff because that one implemented a comparison between the FE_RaviartThomas and FE_RaviartThomasNodal and explicitly used the same polynomial space. However, that is no longer the case so we can't make much sense of it.

@kronbichler
Copy link
Member Author

Unfortunately I had to touch two tests with very large line counts, like hp/create_mass_matrix_05 which has 90k lines. This is due to the different form of the polynomials with the update.

@kronbichler kronbichler force-pushed the update_raviart_thomas_nodal branch 2 times, most recently from f6be788 to 4aa7ce4 Compare July 6, 2021 07:24
@kronbichler
Copy link
Member Author

I now also added a changelog entry.

@kronbichler
Copy link
Member Author

My next step would be to set up the code for a nodal Nedelec element following the same principles. @kkormann and I have the polynomial spaces and basic ingredients ready. Any more comments on this one before we go on?

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of merging, since this restructuring is a big step towards processing RT also in MatrixFree. But I am not sure if replacing the old class by a new class is the right approach. I don't mind; but what is the policy in this regard? Maybe someone else can do the actual merging?

@konsim83
Copy link
Contributor

Sorry for the late reply, I like the refactored RT element and it looks well implemented and tested. Would be cool to have.

Copy link
Member

@drwells drwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this does cause some slight compatibility issues its a net step forward for fixing all the orientation problems (and matrix-free RT will be nice) so lets merge.

@drwells drwells merged commit d2bd28d into dealii:master Jul 16, 2021
@drwells
Copy link
Member

drwells commented Jul 16, 2021

I am in favor of merging, since this restructuring is a big step towards processing RT also in MatrixFree. But I am not sure if replacing the old class by a new class is the right approach. I don't mind; but what is the policy in this regard? Maybe someone else can do the actual merging?

Not being backwards compatible is bad, but - at some point we have to sacrifice it in order to fix bugs. I don't think we have a clear policy but I believe getting the orientation code right is more important than printing the same output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants