-
Notifications
You must be signed in to change notification settings - Fork 167
Closure orientation changes (WIP) #3332
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
Draft
connorjward
wants to merge
6
commits into
master
Choose a base branch
from
orientation-hacks2
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To make this globally consistent, you need to use global section; see
firedrake/firedrake/mesh.py
Line 1092 in d282271
-(o + 1)
rule.We decided to order DoFs on each entity relative to the cone of that entity for checkpointing purpose because the ordering of the cone is guaranteed to be preserved in the save-load cycle.
Modifying cones using
DMPlexOrientPoint()
based on the global vertex numbers is actually not desired as global vertex numbers are not preserved in the save-load cycle (or upon redistribution of the mesh).I think we can make the global vertex numbers (and actually all the global entity numbers) preserved upon save/load or redistribution, but it has yet to be done. Even if it has been implemented, I think, we should abandon all approaches based on the global vertex numbers as hex requires a more generic approach anyway.
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.
Sure. That should be straightforward to add.
I didn't realise this. How come checkpointing works currently then? What I have here is a simplification of the prior approach so the same limitations should apply.
I disagree with this. I think that we want to have this because it allows us to pack/unpack H(div) and H(curl) spaces without potentially expensive runtime transformations.
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.
Another contribution of this PR that I want to emphasise is that it unifies the code paths for
cell_closure
andentity_orientations
. I think we can pretty much rely on the orientations fromgetTransitiveClosure
for our own work (with a single transformation from canonical plex to canonical FIAT).Basically PETSc tells us things like "relative to this cell, this edge is flipped". I believe that, subject to a canonical transformation, this is universal information applicable to both DMPlex and FIAT.
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.
I have thought more on this and this makes a lot of sense. The cones should not be arbitrarily modified upon loading the mesh. However, I still think that my approach has value. Could we introduce perhaps a
DMCopy
of the topology just for tabulating the closure, called, say,closure_dm
? The advantage of having such a DM is then we get nicely ordered closures and can preserve the orientable properties of simplices and quads.