Flatten entire array containers by alexfikl · Pull Request #91 · inducer/arraycontext

alexfikl · 2021-09-26T00:19:04Z

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:

~~Point loopy back to main after Fix strides for 0-sized arrays loopy#497~~ (xfailed the test and leaving this for later)

@thomasgibson Would something like this do what you need?

thomasgibson

This is awesome. Thanks for doing this! I tried them out in inducer/grudge#154 and they appear to work perfectly. I left some comments below, but I'm really keen to have this functionality!

thomasgibson · 2021-09-26T14:52:51Z

+.. autofunction:: flatten_to_numpy
+.. autofunction:: unflatten_from_numpy


Maybe we should try to distinguish {flatten, unflatten}_{to,from}_numpy from their named equivalents in meshmode.dof_array. Though in my opinion, I think that {flatten, unflatten}_{to,from}_numpy makes way more sense here because it's actually flattening the entire container (rather than only flattening components as is the case of meshmode.dof_array.{flatten, unflatten}_{to,from}_numpy.

However, I am fine with just renaming these functions too. How about rec_flatten_to_numpy and similar? rec_ because it recursively flattens?

flatten_container_to_numpy and unflatten_numpy_to_container actually could work too.

Those sound good to me! I'll wait for some more feedback, but otherwise we can rename them like that.

Agree with @thomasgibson's point: We should clarify the relationship between these and the DOFArray flatteners. Given that these do just fine without resorting to the latter, why don't we just deprecate those and define flattening generically, here? Then we wouldn't be obliged to change the name either, since it's clear that these are simply a more generic version of the former. Passing a template seems nicer anyway than passing a discretization.

Agreed. One issue is that they do slightly different things:

these flatten the whole container into one array

the ones in meshmode.dof_array traverse the container and only flatten the DOFArrays.

Not sure if anyone depends on that functionality though? So it could be safe to deprecate and point to these functions instead.

Thanks for spotting that! I don't think anyone is relying on the "just the DOFArrays" aspect of those. (I'm not even sure in what scenario that would be useful.) Plus, if desired, it can be replicated with relative ease atop these new functions.

Yeah, that sounds good! I'll add some deprecation warnings in there after this is in.

inducer

Thanks for working on this! This actually contains some interesting questions about the abstraction as a whole.

inducer · 2021-09-26T22:38:38Z

+.. autofunction:: flatten_to_numpy
+.. autofunction:: unflatten_from_numpy


Agree with @thomasgibson's point: We should clarify the relationship between these and the DOFArray flatteners. Given that these do just fine without resorting to the latter, why don't we just deprecate those and define flattening generically, here? Then we wouldn't be obliged to change the name either, since it's clear that these are simply a more generic version of the former. Passing a template seems nicer anyway than passing a discretization.

inducer

Thanks! I think this on a good path. A few more comments below.

inducer

Thanks! This looks great now. Just two final wrinkles.

inducer

Thanks! Spotted just a few more things. I think those are it though. :)

inducer · 2021-09-29T20:18:35Z

+            iterable = serialize_container(template_subary)
+        except TypeError:
+            # NOTE: the max is needed to handle device scalars with size == 0
+            offset += max(1, template_subary.size)


Upon thinking about it more, I don't think this is right. Device scalars report a size of 1:

>>> import numpy as np >>> np.zeros(()).size 1 >>> a = np.zeros(()) >>> import pyopencl as cl >>> ctx = cl.create_some_context(interactive=False) >>> queue = cl.CommandQueue(ctx) >>> import pyopencl.array >>> a_dev = cl.array.to_device(queue, a) >>> a_dev.size 1

The tests should cover device scalars and arrays that genuinely have zero size (e.g. shape=(3, 0, 4))

Yeah, you're completely right! I removed that and added some tests in 27b894a.

A couple of wiggles there:

for 0-sized arrays, e.g. (18, 0) in the test, when reshaped the strides were (0, 8), but the template had (8, 8). Not sure who's fault that is?

pytato didn't like reshaping scalars or 0-sized arrays. I'll make a small example and complain there.

inducer/pytato#157

For the first part, it seems like it's something in pyopencl.

Hopefully inducer/compyte#36

thomasgibson

I looked through this PR after the latest changes and I think this looks even better. Can also confirm it works like a charm for inducer/grudge#154. I'll let @inducer have a chance to read over it but I wanted to leave my approval now. 😄

inducer

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

alexfikl · 2021-10-07T00:16:30Z

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 · 2021-10-07T00:18:49Z

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

thomasgibson · 2021-10-12T03:58:48Z

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

alexfikl · 2021-10-12T04:03:20Z

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 · 2021-10-12T04:21:15Z

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

thomasgibson · 2021-10-12T16:02:33Z

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

Co-authored-by: Andreas Klöckner <inform@tiker.net>

inducer · 2021-10-18T03:18:09Z

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 · 2021-10-18T03:20:30Z

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 · 2021-10-18T15:30:22Z

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.

alexfikl changed the title ~~Flatten entire array container~~ Flatten entire array containers Sep 26, 2021

alexfikl force-pushed the flatten-to-numpy branch 2 times, most recently from e9450ed to e133b9a Compare September 26, 2021 00:48

thomasgibson reviewed Sep 26, 2021

View reviewed changes

thomasgibson mentioned this pull request Sep 26, 2021

Array container support and batched (eager) communication inducer/grudge#154

Merged

inducer mentioned this pull request Sep 26, 2021

Container serialize/deserialize: Do we need the keys? #92

Closed

inducer reviewed Sep 26, 2021

View reviewed changes

inducer reviewed Sep 27, 2021

View reviewed changes

inducer reviewed Sep 28, 2021

View reviewed changes

Comment thread arraycontext/container/traversal.py Outdated

Comment thread arraycontext/container/traversal.py Outdated

alexfikl force-pushed the flatten-to-numpy branch from 326cc79 to 3ccf485 Compare September 28, 2021 14:55

inducer reviewed Sep 29, 2021

View reviewed changes

alexfikl mentioned this pull request Sep 30, 2021

Reshape on empty arrays inducer/pytato#157

Closed

thomasgibson approved these changes Sep 30, 2021

View reviewed changes

alexfikl mentioned this pull request Oct 1, 2021

Fix strides for 0-sized empty arrays inducer/compyte#36

Merged

alexfikl force-pushed the flatten-to-numpy branch from 42a9a65 to 0bd1b02 Compare October 6, 2021 14:02

inducer approved these changes Oct 7, 2021

View reviewed changes

Comment thread arraycontext/container/__init__.py Outdated

Comment thread arraycontext/container/traversal.py Outdated

Comment thread arraycontext/container/traversal.py Outdated

Comment thread arraycontext/container/traversal.py Outdated

alexfikl force-pushed the flatten-to-numpy branch from 6b86fa3 to 2191fad Compare October 7, 2021 00:59

alexfikl mentioned this pull request Oct 7, 2021

Fix strides for 0-sized arrays inducer/loopy#497

Closed

alexfikl added 6 commits October 15, 2021 19:15

add flatten to numpy for an entire container

917c656

add flatten and unflatten

768a007

mention that serialize_container should be deterministic

3e81dd2

rename argument in unflatten_to_numpy

18e08ab

remove flatten_to_numpy and unflatten_from_numpy

da86855

raise if flattened container does not have homogeneous dtypes

5478997

alexfikl and others added 13 commits October 15, 2021 19:15

update tests with more array container layouts

1d914ab

complain if unflattened strides do not match

d859cd8

improve docs for serialize_container

91e8950

Co-authored-by: Andreas Klöckner <inform@tiker.net>

remove unused memoize_in

8fb7d28

make failed reshape message less opaque

82a8459

hardcode flatten and unflatten in c order

c56e8f5

add tests for flatten edge cases

087351c

slice forwards when unflattening

85c8b39

add missing space in exception message

4d0fe4d

Co-authored-by: Andreas Klöckner <inform@tiker.net>

add some more tests for unflatten

5dc9da2

update test skip condition

7b03174

improve docs

853f501

Co-authored-by: Andreas Klöckner <inform@tiker.net>

unflatten: better check that template and ary sizes match

c826993

Co-authored-by: Andreas Klöckner <inform@tiker.net>

alexfikl force-pushed the flatten-to-numpy branch from 2e6b650 to b0bde7c Compare October 16, 2021 00:15

alexfikl force-pushed the flatten-to-numpy branch from b0bde7c to c826993 Compare October 18, 2021 03:22

alexfikl commented Oct 18, 2021

View reviewed changes

Comment thread arraycontext/impl/pyopencl/fake_numpy.py Outdated

update xfail link

c50ee3e

alexfikl added 3 commits October 18, 2021 16:20

pyopencl: remove unused astype

004e85f

Merge branch 'main' into flatten-to-numpy

f16ccab

Merge branch 'main' into flatten-to-numpy

6bc8d56

thomasgibson merged commit c5ed317 into inducer:main Oct 19, 2021

alexfikl mentioned this pull request Oct 20, 2021

Update DOFArray flatten and unflatten inducer/meshmode#275

Merged

		.. autofunction:: flatten_to_numpy
		.. autofunction:: unflatten_from_numpy

Conversation

alexfikl commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasgibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasgibson Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alexfikl Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inducer left a comment

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!

thomasgibson left a comment

Choose a reason for hiding this comment

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexfikl commented Oct 7, 2021

Uh oh!

inducer commented Oct 7, 2021

Uh oh!

thomasgibson commented Oct 12, 2021

Uh oh!

alexfikl commented Sep 26, 2021 •

edited

Loading

thomasgibson Sep 26, 2021 •

edited

Loading

alexfikl Sep 27, 2021 •

edited

Loading

alexfikl commented Oct 18, 2021 •

edited

Loading