Update usage of {Path,Surface}OrientationInfo #1196
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1196 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 39 39
Lines 6555 6559 +4
=======================================
+ Hits 6164 6168 +4
Misses 391 391 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@AbdAlazezAhmed do you remember what exactly was the reason to not use path and surface orientation info, but introduce a new OrientationInfo struct? I personally prefer a design where we keep orientation information for each dimension separate tho, because this should help developers in understanding what exactly is going on here and what exactly the involved assumptions/invariants for each dimension are. |
|
So the plan from my side was to add different data types per dimension to control internal dispatches, which will be helpful to implement support for problems higher dimensions than 3 later on, as we just need to fill in the places with the dispatches. |
I actually like that better as well. I have no problem implementing that, but would be good to have confirmation about why the separate |
|
Seems like the plan was to replace them was Then it became Then back to |
Not sure how helpful that is for keeping the code complexity manageable. For generic dimensions (=n) the best you can probably do is encoding the orientation directly as an m-dimensional bitvector. |
gijswl
changed the title
|
Are the names deliberately chosen, or should they be updated to match the rest of the terminology:
|
|
I have chosen the names deliberately, as they precede the consistent rename to edge/face and facet/ridge. I have no strong feeling on renaming the orientation info structs. |
I have a strong feeling they should be renamed 😄 |
| """ | ||
| struct PathOrientationInfo | ||
| regular::Bool # Indicator whether the orientation is regular or inverted. | ||
| flipped::Bool # Indicator whether the orientation is regular or inverted. |
There was a problem hiding this comment.
IMO it is less straight forward to store if the edge is flipped, than if it is regular. Personally, I would prefer positive, or perhaps we even can save simply as
direction::Int8 # \pm 1and return Int8 from get_face_direction and get_edge_direction. Probably, we won't see any significant performance difference by using Int8 vs Int, don't recall the profiles of the dof distribution now if this logic takes some significant portion of the time...
There was a problem hiding this comment.
Reagardng the dof distribution time, it is mostly spend in the hash table lookups, as we do not materialize the mesh entities explicitly. But I think just a boolean is clearer here. Also, I would expect that flipped = true would mean 2--->1.
There was a problem hiding this comment.
A "flip" is indeed the terminology used in the Scroggs paper to indicate that the nodes are swapped compared to the "normal" orientation. So,
1 --> 2isflipped = false2 --> 1isflipped = true
This is the same way OrientationInfo already worked and in line with the paper. The previous definition of regular::Bool was opposite, of course. Can you check whether the tests make sense following this definition?
There was a problem hiding this comment.
I see now that the docstring was not updated yet.
Refer to the latest commit for the corrected definition.
4 participants
As far as I can see,
OrientationInfohas the same functionality asPathOrientationInfoandSurfaceOrientationInfo, so it makes more sense to use only the former. This is preparation work for #1195.