PytatoArrayContext.freeze: support container freezes by kaushikcfd · Pull Request #158 · inducer/arraycontext

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


 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)

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.

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


 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.

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


 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.freeze` Co-authored-by: Andreas Kloeckner <inform@tiker.net>

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 a2d3debCompare 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 1bcd6c4Compare 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 2cb8d04Compare 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 7bef0f6Compare May 13, 2022 05:31

kaushikcfd force-pushed the container_freeze branch 4 times, most recently from 20b54ce to 7db55caCompare 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 d373c46Compare June 9, 2022 15:01

inducer reviewed Jun 9, 2022
View reviewed changes

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

inducer reviewed Jun 9, 2022
View reviewed changes

kaushikcfdand 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 <inform@tiker.net>

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

CI: modernize the freezes on DOFArrays
af20c26

kaushikcfd force-pushed the container_freeze branch from cbd3ade to 39b7cc7Compare 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 af20c26Compare June 9, 2022 19:22

inducer merged commit b0d70e6 into inducer:mainJun 9, 2022

Conversation

kaushikcfd commented May 1, 2022• edited by inducer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inducer commented May 6, 2022

Uh oh!

kaushikcfd commented May 9, 2022

Uh oh!

inducer commented May 9, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

inducer commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaushikcfd commented May 1, 2022•
edited by inducer
Loading