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

Inflated geometric factors storage for affine elements #314

Closed
inducer opened this issue Aug 10, 2023 · 5 comments
Closed

Inflated geometric factors storage for affine elements #314

inducer opened this issue Aug 10, 2023 · 5 comments

Comments

@inducer
Copy link
Owner

inducer commented Aug 10, 2023

In #310 (comment), @kaushikcfd says:

I noticed a subtle and sub-optimal behavior that is worth noting. If we are using PyOpenCLActx as a setup actx and PytatoActx as an operational actx, then the frozen geometric factors (such as normals) would be stored at per-DOF even for affine geometry mappings. Such use-cases will bump memory footprint.

If true, this wouldn't just increase memory footprint, but also memory bandwidth requirements.

cc @majosm @matthiasdiener

@majosm
Copy link
Collaborator

majosm commented Aug 10, 2023

I'm told that we don't use a separate array context for init anymore in Y3, so it looks like this isn't affecting us at the moment.

@inducer
Copy link
Owner Author

inducer commented Aug 10, 2023

@kaushikcfd Could you elaborate on the circumstance under which you've observed this? Do you happen to know why this happens?

@kaushikcfd
Copy link
Collaborator

I did not observe this in any production-related drivers. But I was writing some tests for #310 and noticed this behavior and thought sharing would be essential so that others do not repeat the same mistake.

I have attached a minimal reproducer script for posterity.

import grudge
import pyopencl as cl
from meshmode.discretization.connection import FACE_RESTR_INTERIOR

from grudge.array_context import get_reasonable_array_context_class
from meshmode.mesh.generation import generate_regular_rect_mesh

ctx = cl.create_some_context()
cq = cl.CommandQueue(ctx)

ref_actx = get_reasonable_array_context_class(lazy=False, distributed=False)(cq)
actx = get_reasonable_array_context_class(lazy=True, distributed=False)(cq)

mesh = generate_regular_rect_mesh(a=(-0.5, -0.5),
                                  b=(0.5, 0.5),
                                  nelements_per_axis=(16, 16))

dcoll = grudge.make_discretization_collection(ref_actx, mesh, order=4)
nx, ny = actx.thaw(dcoll.normal(FACE_RESTR_INTERIOR))

assert nx[0].shape[-1] == 1  # fails

@inducer
Copy link
Owner Author

inducer commented Aug 11, 2023

Thanks for providing the reproducer. AFAICT, this affects only the normals. The main issue is that here:

return self._setup_actx.freeze(normal(self._setup_actx, self, dd))

the setup actx is unconditionally used to populate the normal cache, making this an interface bug more than a "bug bug". Perhaps the most reasonable response is to deprecate DiscretizationCollection.normal. grudge.geometry.normal takes an actx and does not have this issue (and it's more convenient to use because there's no freeze/thaw cycle needed).

@inducer
Copy link
Owner Author

inducer commented Aug 11, 2023

#315

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

No branches or pull requests

3 participants