inducer / arraycontext

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

`arraycontext.container.to_numpy` semantics #159

Closed kaushikcfd closed 2 years ago

kaushikcfd commented 2 years ago

For the following script:

import pyopencl as cl
from arraycontext import (PytatoPyOpenCLArrayContext as BasePytatoArrayContext,
                          to_numpy)

class PytatoArrayContext(BasePytatoArrayContext):
    def transform_loopy_program(self, t_unit):
        return t_unit

cl_ctx = cl.create_some_context()
cq = cl.CommandQueue(cl_ctx)
actx = PytatoArrayContext(queue=cq)

u = actx.freeze(actx.zeros(10, dtype="float64"))
actx.to_numpy(u)  # Works
to_numpy(u, actx)  # Fails!

I get the following error:

TypeError: array of type 'TaggableCLArray' not in supported types (<class 'pytato.array.Array'>,)

My proposal would be to keep the arraycontext.container.to_numpy's implementation lean by simply calling actx.to_numpy on every leaf array and let the to_numpy method raise TypeError whenever appropriate.

alexfikl commented 2 years ago

As the one responsible for the current code.. Agreed!

inducer commented 2 years ago

IIUC, you're proposing that ArrayContext.{from,to}_numpy should support containers?

kaushikcfd commented 2 years ago

IIUC, you're proposing that ArrayContext.{from,to}_numpy should support containers?

No, just that the logic of what array types are legal to be frozen must be moved the respective implementations. I found this condition: https://github.com/inducer/arraycontext/blob/17dcc60de852f5ff84b81dee68968a80ff22aa69/arraycontext/container/traversal.py#L835 to be quite restrictive, as we couldn't convert cl_arrays to NumPy arrays for PytatoPyOpenCLArrayContext.

alexfikl commented 2 years ago

For my understanding, why doesn't PytatoPyOpenCLArrayContext advertise support for both the frozen and unfrozen array types? i.e. for both pt.Array and cl.Array.

inducer commented 2 years ago

Agree with @alexfikl, that seems like a relatively clean thing to do.

kaushikcfd commented 2 years ago

That was because operations like pt.Array+cla.Array aren't defined.

inducer commented 2 years ago

That was because operations like pt.Array+cla.Array aren't defined.

I think that's alright. array_types just says "these are the possible types that come out of this". It doesn't need to entail a promise of interoperability.

kaushikcfd commented 2 years ago

I agree, I like this. I'll change #160 to implement this. Thanks to both!