search

inducer / arraycontext

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

`with_container_arithmetic`: Default cls_has_array_context_attr based on presence of attribute #155

Closed inducer closed 2 years ago

inducer commented 2 years ago

I think it makes sense for WaveState * pytato.Array to just work, but currently it won't, because with_container_arithmetic will only access .array_context on a container (to get the array types) if _cls_has_array_context_attr is set to True. Just defaulting it to True if the attribute exists also doesn't work, because many of those properties are implemented like this: https://github.com/inducer/arraycontext/blob/b75ba4f60c601664edc495cc3606fc9f7e47866b/test/test_arraycontext.py#L684-L686= but are then used with numpy arrays in the .mass attribute, where that implementation will fail.

If a class has the .array_context attribute, the code in this PR tries harder to get a hold of the array context involved, ultimately nudging people to fix their .array_context property implementations and passing _cls_has_array_context_attr=True (which we may then deprecate even further into the future, using just existence of the attribute instead).

inducer commented 2 years ago

@alexfikl Sorry about the review request... this clearly needs some more thought.

inducer commented 2 years ago

Thanks for taking a look!

This is annoyingly complicated

I totally agree. I kind of hate it. But at least it puts us on a course towards more sanity? At some point? Maybe?

inducer commented 2 years ago

OK, what's the worst that could happen. Let's land it! :) :tada: