search

inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

PytatoArrayContext.freeze: support container freezes #158

Closed kaushikcfd closed 2 years ago

kaushikcfd commented 2 years ago

Freezing across containers can potentially re-use common sub-expressions.

Draft because:

inducer commented 2 years ago

Discussed with @alexfikl a bit regarding the issue of how to set DOFArray.array_context to None as part of a context-driven freeze. While, at the time, we agreed that we should get rid of that attribute entirely (discussion in https://github.com/inducer/arraycontext/issues/162), but that's a total non-starter in the short term for the amount of breakcage it causes (and possibly in the long term, too).

So the second-best solution we came up with is to deprecate arraycontext.container.traversal.{freeze,thaw} and make them aliases of the respective methods in the array context (which are now defined to allow containers). In addition, introduce a new @singledispatch interface arraycontext.container.with_array_context (used by the array context's freeze and thaw) that returns a new array with the corresponding array context stored inside of it.

How does that sound?

kaushikcfd commented 2 years ago

How does that sound?

Yep, this sounds like a cleaner approach. I've implemented the suggestions, however back-compatibility seems challenging (unachievable?). For ex. meshmode needs https://github.com/inducer/meshmode/pull/327.

inducer commented 2 years ago

back-compatibility seems challenging (unachievable?).

Yep, that's understood. But IMO DOFArray is the only container affected (for now), so I think that's OK. This is fixing (papering over?) a genuine design mistake, so I'd rather deal with it than be locked into a dumb interface forever.

inducer commented 2 years ago

LGTM, thanks! Force-merging based on success of previous CI.