search

inducer / arraycontext

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

Deprecate `is_array_container` #106

Closed inducer closed 3 years ago

inducer commented 3 years ago

I think it's confusing because it only looks at the type. That means, for example for non-object numpy arrays, it can give the wrong answer.

cc @majosm @alexfikl

inducer commented 3 years ago

Or maybe rename (with deprecation) to is_array_container_type?

alexfikl commented 3 years ago

Hm, we already have an is_array_container_type (only used by the code generation stuff?) which mostly does the same thing as is_array_container, but takes a class instead of an instance.

https://github.com/inducer/arraycontext/blob/5eec699eb1a81ef82879657b61731ea6a6441f11/arraycontext/container/__init__.py#L155-L172

Replacing is_array_container with "try serializing and if it works it's an array container" (that you mentioned a while back?) makes more sense to me too. A big roadblock is the code generation: how will it know what's an array container and what isn't?

inducer commented 3 years ago

A big roadblock is the code generation: how will it know what's an array container and what isn't?

What usage do you mean? I'm not sure I was able to find anything that didn't look replaceable.

alexfikl commented 3 years ago

What usage do you mean? I'm not sure I was able to find anything that didn't look replaceable.

Something like this?

https://github.com/inducer/arraycontext/blob/5eec699eb1a81ef82879657b61731ea6a6441f11/arraycontext/container/dataclass.py#L52-L55

inducer commented 3 years ago

I think we might be talking past each other. is_array_container_type is just fine IMO. is_array_container is bad IMO because it takes an instance and still gets it wrong.

alexfikl commented 3 years ago

Ah, I see! I was just worried that it created some inconsistencies in determining what's a container. Although we have some of those already, since is_array_container can be True and serializing fails..

Either way, definitely :+1: to deprecating is_array_container!

inducer commented 3 years ago

it created some inconsistencies in determining what's a container

The goal is the opposite! I'd like to remove the inconsistency that is_array_container can say "yes, a numpy array is a container", but then whoops serialization fails. At any rate, I'll PR a deprecation.