Skip to content
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

Periodic vertexonlymesh #2437

Merged
merged 11 commits into from May 20, 2022
Merged

Periodic vertexonlymesh #2437

merged 11 commits into from May 20, 2022

Conversation

dham
Copy link
Member

@dham dham commented May 11, 2022

This PR swaps the order of mesh searching in the VertexOnlyMesh setup so that we do the Firedrake search for parent mesh cell inclusion before the PETSc one. We then use this to set up the swarm coordinates instead of calling swarm.setPointCoordinates. This means that we're not dependent on Firedrake and DMPlex agreeing on how coordinates work.

This needs a little cleanup and docstrings, and I need to do the parallel bounding box elimination (which isn't hard). I think the extension to extrusion is probably fairly straightforward and I think I might know how to do bendy.

@ReubenHill
Copy link
Contributor

I believe this is doing the "Quick" solution suggested in #2185 .

@dham
Copy link
Member Author

dham commented May 12, 2022

I believe this is doing the "Quick" solution suggested in #2185 .

Sure, and I think that's good. It's a small amount of work which makes a lot of things work that don't currently. I don't think it's at all worse than what we're currently doing, and I don't think we should refrain from improving the current situation in favour of a potential future fix. If we ever get there then reversing this change is very easy.

firedrake/mesh.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ReubenHill ReubenHill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some changes but if this works without significant performance losses at VertexOnlyMesh creation time then great

firedrake/mesh.py Outdated Show resolved Hide resolved
@ReubenHill ReubenHill self-requested a review May 18, 2022 09:06
Copy link
Contributor

@ReubenHill ReubenHill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good pending some tidying on the commit history and adding a small amount of documentation. I hope the tests pass!

firedrake/mesh.py Outdated Show resolved Hide resolved
firedrake/mesh.py Show resolved Hide resolved
@dham dham marked this pull request as ready for review May 20, 2022 04:13
@dham dham dismissed ReubenHill’s stale review May 20, 2022 04:30

Changes made as requested.

@dham dham merged commit e6e778a into master May 20, 2022
@dham dham deleted the periodic_vertexonlymesh branch May 20, 2022 06:10
@ReubenHill
Copy link
Contributor

ReubenHill commented May 20, 2022

Too late to fix it now, but I note you merged before cleaning up the commit history which was a requested change - things like squashing flake8 and lint into the previous commits Disregard - this was squash merged

@ReubenHill
Copy link
Contributor

Great that this all passed tests and works though! Thanks for the PR 😃

ReubenHill added a commit that referenced this pull request Jan 18, 2023
Since we switched to doing cell location and point location using
_parent_mesh_embedding, which relies on firedrake's notion of cells and
point locations as opposed to using PETSc's DMPlex embedding via
DMSwarmSetPointCoordinates (PR #2437), we ought to be able to support
extruded meshes. After all, .at works on extruded meshes!
ReubenHill added a commit that referenced this pull request Feb 1, 2023
Since we switched to doing cell location and point location using
_parent_mesh_embedding, which relies on firedrake's notion of cells and
point locations as opposed to using PETSc's DMPlex embedding via
DMSwarmSetPointCoordinates (PR #2437), we ought to be able to support
extruded meshes. After all, .at works on extruded meshes!
ReubenHill added a commit that referenced this pull request Feb 8, 2023
Since we switched to doing cell location and point location using
_parent_mesh_embedding, which relies on firedrake's notion of cells and
point locations as opposed to using PETSc's DMPlex embedding via
DMSwarmSetPointCoordinates (PR #2437), we ought to be able to support
extruded meshes. After all, .at works on extruded meshes!
ReubenHill added a commit that referenced this pull request Feb 15, 2023
Since we switched to doing cell location and point location using
_parent_mesh_embedding, which relies on firedrake's notion of cells and
point locations as opposed to using PETSc's DMPlex embedding via
DMSwarmSetPointCoordinates (PR #2437), we ought to be able to support
extruded meshes. After all, .at works on extruded meshes!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants