-
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
Implement FaceQuadratureRule for RefPrism and RefPyramid #779
Implement FaceQuadratureRule for RefPrism and RefPyramid #779
Conversation
…ment it out then forget to delete it and make a new commit for deleting it and get embarassed
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #779 +/- ##
==========================================
+ Coverage 91.88% 92.59% +0.71%
==========================================
Files 33 33
Lines 4916 4942 +26
==========================================
+ Hits 4517 4576 +59
+ Misses 399 366 -33
☔ View full report in Codecov by Sentry. |
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.
Cool!
I just took a quick look through and shared some thoughts (but couldn't look longer as my laptop is running out of battery:)
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.
transfer_point_face_to_cell
could perhaps be called reference_face_to_face
or something?
Nice work on this PR. Before this PR, the spatial coordinate evaluated on "cell-level" was not always the same as the coordinate evaluated on "face level" In other words, we just took the parametric coordinate on e.g. on a RefLine (say \xi=[0.123]) and transformed in to the Quadrilateral (say to local faceid 4 \xi = [-1, 0.123), however, this would not always correspond to the same spatial coordiante due to the permutation of the face-nodes. This PR seems to fix that. Is it worth adding test for this (I can do it in that case)? Somethink like:
Edit: I noticed that you mentioned this in #743 "syncing/transforming quadrature points" |
Thanks @lijas ! I'll work on adding the test 😄 |
Closes #772
Changes the transformations used in
create_face_quad_rule
to use the ones in FerriteViz as they're beneficial for optimizing #743 and IIUC make more sense in general as they respect the normals directions.Also, the transformation is separated into its own functions such that they can be reused as in #743.
Edit: test coverage should improve with interface test, because it covers face->cell transformations