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

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

Open
inducer opened this issue May 6, 2022 · 8 comments
Open

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

inducer opened this issue May 6, 2022 · 8 comments

Comments

@inducer
Copy link
Owner

inducer commented May 6, 2022

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 #158).

What does it break?

  • grudge gets its array context from the arrays it's passed for many operations.
  • Any function that's actx.compiled, because that's its sole source of array cotnext.

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

@alexfikl @kaushikcfd @majosm Thoughts?

@majosm
Copy link
Collaborator

majosm commented May 9, 2022

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
Copy link
Owner Author

inducer commented May 9, 2022

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
Copy link
Collaborator

majosm commented May 9, 2022

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
Copy link
Owner Author

inducer commented May 9, 2022

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
Copy link
Collaborator

majosm commented May 10, 2022

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
Copy link
Owner Author

inducer commented May 10, 2022

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
Copy link
Owner Author

inducer commented Jun 10, 2022

  • 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
Copy link
Owner Author

inducer commented Jul 16, 2024

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

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

2 participants