Flatten entire array containers

inducer / arraycontext

Choose your favorite numpy-workalike!

6 stars 11 forks source link

Flatten entire array containers #91

Closed alexfikl closed 3 years ago

alexfikl commented 3 years ago

Adds two functions, flatten and unflatten, that go between an array container and a 1D array (as supported by the array context).

The couple of gotchas here:

serialize_container and deserialize_container should not be doing anything crazy, i.e. two calls will return the components in the same order, which is the case for all the containers we have.
As it doesn't impose its own order, it's not safe for long time storage (i.e. in case something changes order in serialiaze_container), but that shouldn't be too much of an issue.
Naming? The same functions in meshmode basically just flatten DOFArrays and leave the rest of the array container the same.

Before merging:

[x] ~~Point loopy back to main after inducer/loopy#497~~ (xfailed the test and leaving this for later)

@thomasgibson Would something like this do what you need?

alexfikl commented 3 years ago

LGTM, aside from these final wrinkles. If you're OK with accepting all these suggestions, then this is ready to go from my end.

They look good to me! This still depends on the fixes from inducer/compyte#36 and inducer/pyopencl#514 to get the tests to pass without the added xfail. We can remove that later though.

inducer commented 3 years ago

Those are close, too. So let's just wait on them.

thomasgibson commented 3 years ago

What's the status of this PR? Are there still issues remaining?

alexfikl commented 3 years ago

What's the status of this PR? Are there still issues remaining?

The general case should work as advertised. We hit an edge case with arrays that have a 0-size in one of the axes and don't work quite yet on pytato due to inducer/loopy#497.

inducer commented 3 years ago

And I've been just slow to review. Sorry!

thomasgibson commented 3 years ago

No worries! I just wanted to make sure there wasn't a "gotcha" with this change

inducer commented 3 years ago

I wonder whether we should just xfail the loopy-dependent zero-size bits for now and bump them to an issue, so @thomasgibson can start using this... what do you all think?

alexfikl commented 3 years ago

I wonder whether we should just xfail the loopy-dependent zero-size bits for now and bump them to an issue, so @thomasgibson can start using this... what do you all think?

Yeah, definitely agree with that. The loopy fix might take a while and it's not breaking any actual code at the moment, just an edge case.

EDIT: Updated the url in c50ee3e to point to the loopy PR.

thomasgibson commented 3 years ago

I wonder whether we should just xfail the loopy-dependent zero-size bits for now and bump them to an issue

Yes, sounds good to me.