PytatoArrayContext.freeze: support container freezes #158

kaushikcfd · 2022-05-01T18:23:43Z

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

⚠️ Needs to go in conjunction with Support with_array_context for DOFArrays meshmode#327.

Draft because:

Pass CI by correctly pointing the meshmode branch in the downstream CI.
DROP final commit before merge

inducer · 2022-05-01T19:33:49Z

arraycontext/container/traversal.py

@@ -533,6 +533,9 @@ def freeze(

    See :meth:`ArrayContext.thaw`.
    """
+    if actx is not None and actx.supports_freezing_containers:


We should just deprecate these then (and expect all array contexts to support container freezes).

Given that the freeze logic is specific to the containers (freeze is a single-dispatch function), deprecating this won't be possible.

This should also be added to meshmode.dof_array then?

As a side note, I really don't recall why we made this singledispatch-ed and can't think of a reason for it now. Anyone remember something?

I don't. Array container freeze seems completely generic.

I really don't recall why we made this singledispatch-ed and can't think of a reason for it now. Anyone remember something?

IIUC, array container initialization might be specific to the container type, and instead of forcing any constructor API we ask the container's writer to also provide a freeze. For example, DOFArray takes in a .array_context argument during initialization that must be set to None on getting frozen.

This should also be added to meshmode.dof_array then?

Yep!

IIUC, array container initialization might be specific to the container type, and instead of forcing any constructor API we ask the container's writer to also provide a freeze. For example, DOFArray takes in a .array_context argument during initialization that must be set to None on getting frozen.

And using deserialize_container doesn't work because the template arg isn't necessarily frozen. Hm.. annoying..

Thanks for the reminder!

Revisiting this: given the new logic, I think we ought to just deprecate the stand-alone freeze in favor of actx.freeze. (same for thaw) I don't see a strong rationale for keeping them.

They do make it easy to just call freeze(container) as the actx is automatically inferred, if possible. If this is something that seems unnecessary, happy to deprecate it.

In view of #162, I'd prefer to reduce our dependence on these, so deprecating them now seems like a good move. I know that this is will be super noisy on the application end, but I think it's a necessary (if painful) step.

I'd welcome feedback though. (cc @MTCam)

arraycontext/container/traversal.py

inducer · 2022-05-06T23:51:29Z

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 #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 · 2022-05-09T22:48:28Z

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 inducer/meshmode#327.

inducer · 2022-05-09T23:49:19Z

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.

arraycontext/container/traversal.py

inducer · 2022-06-09T17:12:50Z

arraycontext/container/traversal.py

@@ -533,6 +533,9 @@ def freeze(

    See :meth:`ArrayContext.thaw`.
    """
+    if actx is not None and actx.supports_freezing_containers:


Revisiting this: given the new logic, I think we ought to just deprecate the stand-alone freeze in favor of actx.freeze. (same for thaw) I don't see a strong rationale for keeping them.

inducer

Thanks! A few things to do here, but hopefully nothing onerous or disruptive. LGTM once these are take care of.

arraycontext/container/traversal.py

inducer · 2022-06-09T17:34:59Z

arraycontext/container/traversal.py

@@ -533,6 +533,9 @@ def freeze(

    See :meth:`ArrayContext.thaw`.
    """
+    if actx is not None and actx.supports_freezing_containers:


In view of #162, I'd prefer to reduce our dependence on these, so deprecating them now seems like a good move. I know that this is will be super noisy on the application end, but I think it's a necessary (if painful) step.

I'd welcome feedback though. (cc @MTCam)

arraycontext/container/traversal.py

…ArrayContext.freeze` Co-authored-by: Andreas Kloeckner <[email protected]>

inducer · 2022-06-09T19:23:02Z

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

inducer reviewed May 1, 2022

View reviewed changes

kaushikcfd force-pushed the container_freeze branch 2 times, most recently from 9877625 to a2d3deb Compare May 2, 2022 14:24

This was referenced May 2, 2022

Issues with zeros et al functions of array contexts #161

Open

Make it so array containers do not know their array context? #162

Open

kaushikcfd force-pushed the container_freeze branch 3 times, most recently from a430a74 to 1bcd6c4 Compare May 9, 2022 21:47

kaushikcfd mentioned this pull request May 9, 2022

Support with_array_context for DOFArrays inducer/meshmode#327

Merged

1 task

kaushikcfd force-pushed the container_freeze branch from 1bcd6c4 to 2cb8d04 Compare May 12, 2022 17:47

kaushikcfd mentioned this pull request May 12, 2022

Freeze current state before storing illinois-ceesd/drivers_y2-isolator#22

Merged

kaushikcfd force-pushed the container_freeze branch 2 times, most recently from 70d61a2 to 7bef0f6 Compare May 13, 2022 05:31

kaushikcfd force-pushed the container_freeze branch 4 times, most recently from 20b54ce to 7db55ca Compare June 8, 2022 05:15

kaushikcfd marked this pull request as ready for review June 8, 2022 06:24

kaushikcfd requested a review from inducer June 8, 2022 06:25

kaushikcfd force-pushed the container_freeze branch from 7db55ca to d373c46 Compare June 9, 2022 15:01

inducer reviewed Jun 9, 2022

View reviewed changes

kaushikcfd force-pushed the container_freeze branch from d373c46 to cbd3ade Compare June 9, 2022 17:29

inducer reviewed Jun 9, 2022

View reviewed changes

kaushikcfd and others added 3 commits June 9, 2022 13:10

restructure the way 'freeze' is implemented. Move the major bits to `…

39e3352

…ArrayContext.freeze` Co-authored-by: Andreas Kloeckner <[email protected]>

to_numpy: thaw(freeze(...)) first

3296e19

CI: modernize the freezes on DOFArrays

af20c26

kaushikcfd force-pushed the container_freeze branch from cbd3ade to 39b7cc7 Compare June 9, 2022 18:10

kaushikcfd requested a review from inducer June 9, 2022 18:16

inducer force-pushed the container_freeze branch from 39b7cc7 to af20c26 Compare June 9, 2022 19:22

inducer merged commit b0d70e6 into inducer:main Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PytatoArrayContext.freeze: support container freezes #158

PytatoArrayContext.freeze: support container freezes #158

kaushikcfd commented May 1, 2022 •

edited by inducer

Loading

inducer May 1, 2022

kaushikcfd May 2, 2022

alexfikl May 2, 2022

alexfikl May 2, 2022

inducer May 2, 2022

kaushikcfd May 2, 2022

alexfikl May 2, 2022

inducer Jun 9, 2022

kaushikcfd Jun 9, 2022

inducer Jun 9, 2022

inducer commented May 6, 2022

kaushikcfd commented May 9, 2022

inducer commented May 9, 2022

inducer Jun 9, 2022

inducer left a comment

inducer Jun 9, 2022

inducer commented Jun 9, 2022

PytatoArrayContext.freeze: support container freezes #158

PytatoArrayContext.freeze: support container freezes #158

Conversation

kaushikcfd commented May 1, 2022 • edited by inducer Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inducer commented May 6, 2022

kaushikcfd commented May 9, 2022

inducer commented May 9, 2022

Choose a reason for hiding this comment

inducer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inducer commented Jun 9, 2022

kaushikcfd commented May 1, 2022 •

edited by inducer

Loading