-
Notifications
You must be signed in to change notification settings - Fork 89
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
Quadratic Serendipity Hex Interpolation #353
Conversation
Codecov Report
@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 88.19% 88.70% +0.51%
==========================================
Files 20 20
Lines 2219 2284 +65
==========================================
+ Hits 1957 2026 +69
+ Misses 262 258 -4
Continue to review full report at Codecov.
|
Nice! VTK calls there 20-node hexahedron "Quadratichexahedron" (https://vtk.org/doc/nightly/html/classvtkQuadraticHexahedron.html) If I remember correctly, I think we have renumbered our edges differently compared to VTK (we should compare with the numbering in the first link), which would cause output issues in paraview/vtk (but I dont think this is a problem atm since we have no mesh_generator generating 20-node Hexahedrons). Could be good to add test here aswell?:
|
OK! Then let's stick to their names. I was just wondering, because I want to use it for some quadratic hex mesh and when I meshed with gmsh, I just noticed that their default seems to be the TriQuadratic one, while ours now defaults to the 20 node version.
This would be ideal, but for this I'd need a W.r.t. the
|
I'm asking because Ferrite.jl/src/interpolations.jl Line 351 in 8dc3be3
defines at least the first face clockwise and for what I've seen in the repo usually it's defined anti clockwise. Maybe it doesn't matter in the end, I'm just confused |
The order of the face-dofs is not important anywhere in the code right now (i think). But ideally it should be ordered in the same way as we distribute dofs: Corners/vertices, edges, faces and inner celldofs. So the way you have ordered it now looks good to me. Side note: For full compatibility with vtk, we should follow this, but since we never use such high-order interpolations we dont really have to think about it. |
For testing you could just generate a mesh with a few cells by manually giving the nodes and cells. The |
Thanks for the nice ressource, I didn't get it in the beginning till I noticed it's cubic stuff. But I, too, think this is aligned with my ordering now
In @lijas mentioned testset we are looping over the cells and invoke on each cell the I'd say what's left now:
I tested it locally with a small simulation, seems to work fine :) |
Here is a grid generator that seems to work :) I noticed that our edge order does not follow vtk. but vtk has: bottom horizontal edges -- top horizontal edges -- vertical mid edges How is the order of the dofs for your interpolation (on the edges)?
|
Very nice! Thank you!
So, should we follow the vtk definition?
The ordering follows this graphic |
yeah, that way we dont have to modify anything for the outputting, just add Ferrite.cell_to_vtkcell(::Type{QuadraticHexahedron}) = VTKCellTypes.VTK_QUADRATIC_HEXAHEDRON |
…x[], added export and export test
ok, done. I included the test you've mentioned earlier which checks some sha sum. I have overwritten the cheksums.sha1 file now, since the QuadraticHexahedron wasn't there. I just set the overwrite variable to true and afterwards again to false. Was this the correct way? Other than that I'm going to check one last time with a computation if the export is correct |
Nice :) Why did you change FaceIndex to Tuple{Int,Int} in the gridgenerator? All other generators uses FaceIndex. |
short version: derp long version: I branched out before pulling, so I was on a relatively old master commit and therefore I wondered, why FaceIndex is not working and why you would use it in the first place 😄 . Anyhow, I think it's fine now |
Let me know if it's OK to squash and merge or if further things need to be changed :) |
Looks good to me :) Idk know if anyone else wants to take a look... |
Question on this: Does anyone know what the relation between the different VTK hexahedrons is? A major point which I see as problematic is the naming, as we suggest to provice a "second order Lagrange hexahedron" but deliver some denegerate basis with order < 2. I would suggest to leave the "Lagrange dim 3 RefCube order 2" as the usual tensor product element of second order Lagrange basis functions. Other than the naming I like the implementation. |
Aw yes, sorry Maxi, I derped here. You are right, the 20 node hex is a quadratic serendipity element. :) Not sure about the 24 node thingy tho. Some infrastructure is there. However now we run into some mesh issue again. I think it makes sense to introduce some "serendipity geometry" like |
I think this is already possible, in the same way as we allow quadratic interpolation on linear elemets. No need for RefSerendipityCube. |
But yeah, maybe call this interpolation serendipity instead, since there seems to be literature that calls it that :) |
ye there is no need to introduce something new
and the cell stays |
Hmm... Maybe we can keep this un-aliased, and use Cell{3,20,6} for now. Unless someone comes up with a better name. |
Ferrite.jl/src/interpolations.jl Line 428 in 01cf5e1
Is this correct? Should have been wrong before I guess, but didn't test with face integrals where it probably matters?
Yes I think full quadratic hexas should get the name. Un-aliased seems like the best option for now |
I dont think |
cool, even better :D Still, I think this should be the correct dispatch in case anything needs it in the future |
@koehlerson This should be correct. The faces miss the central dof, which corresponds to the quadratic quadrilateral (serendipity element). |
So, I removed the alias and renamed the interpolation. I think this is again ready for review |
Looks good to me. Always good to do some manual test as well, for example try solve the heat equation we have in the example and see that the result is reasonable. |
Any other comments/suggestions or can I merge this? |
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.
LGTM
Edit: Maybe we should change the title to Quadratic Serendipity Hex Interpolation for cleanness.
Added quadratic hex interpolation, although I'm not 100% sure about the
faces
dispatch, in particular I'm not sure if my numbering is correct with the new quadratic nodes.Other than that, I think
QuadraticHexahedron
should be renamed, because its an incomplete 20 node version and the complete one has 27 nodes. If you agree to this, then I think the new interpolation should go under Serendipity.Out of curiosity: is there any advantage of the 20 the node version compared to the 27 node one?