inducer / arraycontext

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

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

Open inducer opened 2 years ago

inducer commented 2 years ago

This is aimed particularly at DOFArray.array_context, which is AFAIK the only instance of this, but a pervasive one. This change, if implemented, is rather break-the-world-y, so we would have to do this slowly and carefully.

Why is it worth doing? It's forcing arraycontext.container.traversal.{freeze,thaw} to be @singledispatch over the container type, just for a way to remove the stored .array_context. For batch-freezing in the array context, this is inconvenient (see discussion in https://github.com/inducer/arraycontext/pull/158).

What does it break?

That seems like a lot of breakage for limited gain...

@alexfikl @kaushikcfd @majosm Thoughts?

majosm commented 2 years ago

Would the individual arrays still know their array context? Or would the array context need to be passed into every function as a separate argument?

inducer commented 2 years ago

Would the individual arrays still know their array context?

They already do not. pyopencl.array.Array has no idea what an array context is. At some point, a numpy array will also be a valid array. It, too, has never heard of an array context. (IOW, we can't be that intrusive.)

majosm commented 2 years ago

Oh right, I forgot. Yeah, this change sounds like it might be rough. Can't say I understand the @singledispatch issue well enough to say whether it's worthwhile or not.

I don't suppose it would be feasible to make a wrapper layer for thawed arrays? i.e. thaw returns something that contains an array and behaves like one but also holds an array_context reference?

inducer commented 2 years ago

I don't suppose it would be feasible to make a wrapper layer for thawed arrays? i.e. thaw returns something that contains an array and behaves like one but also holds an array_context reference?

Given the amount of isinstance(..., something_like_an_array) flying around, I feel like that would be even more destructive than what's in the OP...

majosm commented 2 years ago

Given the amount of isinstance(..., something_like_an_array) flying around, I feel like that would be even more destructive than what's in the OP...

Do we do that a lot? Most of the isinstances I can think of check for containers like DOFArray, not the underlying array types themselves. I was picturing wrapping the underlying arrays, and then using get_array_context_recursively to retrieve an array context for containers like DOFArrays, etc. Again, I definitely don't know the full scope of array context usage well enough to say if something like this makes any sense, I'm just throwing out ideas. Feel free to ignore. 🙂

inducer commented 2 years ago

I appreciate your pariticipating in the discussion! Mostly, I'm not too optimistic about these wrapper-y approaches based on past experience, because there always seem to be hard-to-fix corner cases where the wrapper only imperfectly pretends to be the underlying array. I agree that that's not a very tangible criticism of the approach. :)

inducer commented 2 years ago
  • Any function that's actx.compiled, because that's its sole source of array cotnext.

Upon further thought, I think this one is a non-issue, as users can pass in an appropriate array context via partial or a closure.

inducer commented 3 months ago

FWIW, I'm more or less forcing a decision on this at https://github.com/inducer/grudge/pull/353/files#r1679781502.