Fix map_array_container on non-ArrayContainers #90
Conversation
alexfikl
force-pushed
the
map-container-scalars
branch
from
ba17574
to
4013c54
Compare
arraycontext/container/traversal.py
Outdated
| except NotImplementedError: | ||
| raise ValueError( | ||
| f"Non-array container type has no key: {type(ary).__name__}") |
There was a problem hiding this comment.
I think we may want to distinguish between ValueError and NotImplementedError. Also, I'm not sure if we necessarily need to catch exceptions if we do proper error handling further down
There was a problem hiding this comment.
Not quite sure I follow what the suggestion is. Can you elaborate a bit? This code should be the same as before, it just uses try .. except instead of is_array_container.
There was a problem hiding this comment.
@alexfikl I think the discussion in this thread #90 (comment) settles this!
There was a problem hiding this comment.
Got it! 😁
Thanks a lot for taking a look and sparking a discussion! Always appreciated!
alexfikl
force-pushed
the
map-container-scalars
branch
from
baaab74
to
eda7187
Compare
|
LGTM! I'll go ahead and merge, since I don't think there are any total dealbreakers in here. If there are lingering concerns (@thomasgibson?), please bump them to an issue. |
map_array_containerwas usingtry ... exceptto check if an object is serializable, but it was only catchingTypeError, notNotImplementedErrorthrown byserialize_container. That basically made it try to deserialize floats and other non-array container objects.The
try ... exceptwas introduced in #31, seemingly for some performance reasons (?), so I went ahead and used the same pattern in several other places.@thomasgibson as this is (now, vaguely) related to inducer/grudge#154.