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

Add mesh boundary gluing (aka periodic) #204

Closed
wants to merge 11 commits into from

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented Jun 2, 2021

No description provided.

@majosm
Copy link
Collaborator Author

majosm commented Jun 2, 2021

So I've managed to get periodicity at least partially working.

It seems to work fine with meshes for which the boundary transformation is just a translation, e.g.:

periodic

But, for more general transformations I'm seeing some issues (reflections, instability):

periodic1

Driver code for these cases is here.

As far as I can tell, the transformations are implemented correctly (the Gauss-Newton stuff in _make_cross_face_batches is converging, at least). I think what's causing this is the fact that for most of our operators, we need to exchange fluxes that are vectors; so in the general transformation case we would need to transform these vectors accordingly when going from one periodic boundary to the other, right? I'm not entirely sure how we could go about doing that, or if that's something we could even realistically do. Any thoughts @inducer?

@inducer
Copy link
Owner

inducer commented Jun 2, 2021

Oh cool. Yes, you're definitely right that the numerical flux would need to play a role in the case of more general transformations. (As in, if you give a numerical flux on the + side a normal that's orthogonal to the one on the - side, it's not a huge surprise that nothing good will come of that. I'm kind of surprised you got the simulation to survive as long as you did. :) Making this work right will require some design decisions, such as how to expose such an interface to the numerical flux. The easiest thing would be to expose the coordinate transform in a trace pair and separate out the periodic part of the boundary in a separate one. If we don't separate out that flux exchange, then we'd be paying to apply a pointless identity transform at every flux exchange point. I'm having a hard time guessing whether that would be a big deal in terms of cost.

At any rate, my suggestion would be to devote the rest of this PR to get this merged as is, and to worry about those design decisions in a separate issue. (inducer/grudge#109)

cc @thomasgibson @lukeolson for input

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! Some review bits that came up during our meeting.

meshmode/discretization/connection/opposite_face.py Outdated Show resolved Hide resolved
meshmode/mesh/__init__.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner

inducer commented Jun 3, 2021

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@majosm majosm force-pushed the mesh-boundary-gluing branch 2 times, most recently from 8bd6bc0 to 3308b16 Compare June 14, 2021 17:11
@majosm
Copy link
Collaborator Author

majosm commented Jun 14, 2021

Question that just came up while talking to @thomasgibson: what should happen if someone tries to call map_mesh on a mesh that has nontrivial transforms in its facial adjacency?

(I haven't thought about this at all yet; just putting it here so I don't forget.)

@majosm majosm force-pushed the mesh-boundary-gluing branch 2 times, most recently from 37fb9bb to 536f514 Compare June 22, 2021 15:42
@majosm majosm force-pushed the mesh-boundary-gluing branch 2 times, most recently from 6cedb65 to 70d53e6 Compare June 22, 2021 17:20
@majosm majosm marked this pull request as ready for review June 22, 2021 18:19
@majosm
Copy link
Collaborator Author

majosm commented Jun 22, 2021

I think this is ready for a look @inducer. (There's still a test failure, but it appears to be a pylint false positive?)

@majosm majosm requested a review from inducer June 22, 2021 18:21
@@ -69,6 +69,8 @@
.. autofunction:: generate_box_mesh
.. autofunction:: generate_regular_rect_mesh
.. autofunction:: generate_warped_rect_mesh
.. autofunction:: generate_annular_cylinder_slice_mesh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get a generate_periodic_rect_mesh pretty please? 😃

@@ -42,7 +42,8 @@ def thaw_to_numpy(actx, array):
def _make_cross_face_batches(actx,
tgt_bdry_discr, src_bdry_discr,
i_tgt_grp, i_src_grp,
tgt_bdry_element_indices, src_bdry_element_indices):
tgt_bdry_element_indices, src_bdry_element_indices,
tgt_aff_transforms=None, src_aff_transforms=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Add docstring? (Shape of src_aff_transforms is how I got there.)

@majosm
Copy link
Collaborator Author

majosm commented Sep 23, 2021

Superseded by #253.

@majosm majosm closed this Sep 23, 2021
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