-
Notifications
You must be signed in to change notification settings - Fork 11
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
Comments
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? |
They already do not. |
Oh right, I forgot. Yeah, this change sounds like it might be rough. Can't say I understand the I don't suppose it would be feasible to make a wrapper layer for thawed arrays? i.e. |
Given the amount of |
Do we do that a lot? Most of the |
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. :) |
Upon further thought, I think this one is a non-issue, as users can pass in an appropriate array context via |
FWIW, I'm more or less forcing a decision on this at https://github.com/inducer/grudge/pull/353/files#r1679781502. |
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.actx.compile
d, because that's its sole source of array cotnext.That seems like a lot of breakage for limited gain...
@alexfikl @kaushikcfd @majosm Thoughts?
The text was updated successfully, but these errors were encountered: