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

Overintegration support for operators and reductions #172

Closed
wants to merge 60 commits into from

Conversation

thomasgibson
Copy link
Collaborator

@thomasgibson thomasgibson commented Oct 8, 2021

This PR extends the mass inverse operator to work with overintegration, updates documentation, and extends the norm/integral routines to use quadrature discretizations.

Let's please merge #154 first:

@thomasgibson thomasgibson force-pushed the thg/quad-aware-ops branch 2 times, most recently from fe4926b to 6623fab Compare October 12, 2021 03:27
@thomasgibson thomasgibson changed the title Mass operator support for overintegration Overintegration support for operators and reductions Oct 12, 2021
@thomasgibson thomasgibson marked this pull request as ready for review October 12, 2021 03:53
@thomasgibson thomasgibson force-pushed the thg/quad-aware-ops branch 2 times, most recently from 909d387 to 91081ce Compare October 13, 2021 00:50
Comment on lines +418 to +430
result = dcoll._base_to_geoderiv_connection(dd_base)(result)

if dd.uses_quadrature():
# NOTE: We do not want to interpolate to the quadrature grid
# unless we are using non-affine storage
# (or if *_use_geoderiv_connection* is *False*)
if not dcoll._has_affine_groups() or not _use_geoderiv_connection:
result = dcoll.connection_from_dds(dd_base, dd)(result)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@inducer I think this achieves what we talked about earlier today (to address issue #174)! This appears in two other places (area_elements and mv_normal). Technically 3 if you count shape_operator, but that was just missing an interpolation step!

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure I buy this logic. This suppresses the interpolation to the quadrature grid if there aren't any affine groups and _use_geoderiv_connection is true. Why? If this function gets asked to interpolate, it should.

grudge/op.py Outdated Show resolved Hide resolved
@thomasgibson thomasgibson self-assigned this Oct 14, 2021
@thomasgibson thomasgibson force-pushed the thg/quad-aware-ops branch 2 times, most recently from 430c87a to 33f9702 Compare October 14, 2021 02:48
Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Looked a bit to see #174 and that looks good to me! Left a few unrelated nitpicks along the way 😁

grudge/geometry/metrics.py Outdated Show resolved Hide resolved
grudge/op.py Outdated Show resolved Hide resolved
grudge/op.py Outdated Show resolved Hide resolved
grudge/reductions.py Outdated Show resolved Hide resolved
Base automatically changed from thg/array-containers to main December 6, 2021 23:02
@thomasgibson
Copy link
Collaborator Author

Closing this since I think it's not really necessary, and the important fix related to #174 has been moved into a separate PR: #194

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.

DiscretizationCollection._base_to_geoderiv_connection is broken when using overintegration in lazy-mode
3 participants