-
Notifications
You must be signed in to change notification settings - Fork 115
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
p4est surface performance 3D #783
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.
Overall I think this looks good! The performance improvements are again very impressive - thanks a lot for taking care of this!
My main concern with my questions/remarks is that it becomes much harder to understand what's going compared to before. Thus I think it would be great if we can expand the documentation a little more (especially for the new auxiliary methods) - although I am not sure exactly how and where this should go.
I also think it would be good if @andrewwinters5000 could have a look, since he is the expert on the indexing. Although I tried to understand the logic of the new indexing, I was not able to check everything.
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.
It looks quite nice, thanks @ranocha for taking on the 3D cases! The indexing herein should account for all the different flip configurations of the different surfaces. It makes sense that these index flip checks are done on the fly because the mesh is dynamic. So an element's neighbours and their orientations need this extra machinery to be correct. We can keep this in mind when extending the UnstructuredMesh2D
to 3D
where this index flipping and logic can be done in the mesh constructor once.
Thanks, @andrewwinters5000 👍 |
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!
Codecov Report
@@ Coverage Diff @@
## main #783 +/- ##
==========================================
- Coverage 93.60% 93.54% -0.06%
==========================================
Files 182 182
Lines 17797 17932 +135
==========================================
+ Hits 16658 16774 +116
- Misses 1139 1158 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I ported the 2D surface performance improvements for the
P4estMesh
of #767 to 3D.Current
main
(after compilation, usingjulia --check-bounds=no --num-threads-1
):This PR:
CC @efaulhaber
Closes #642