Conversation
| # FIXME: not clear this is the right default? | ||
| if particle_id_dtype is None: | ||
| particle_id_dtype = np.dtype(np.int32) |
There was a problem hiding this comment.
This seems to be used below unconditionally in some VectorArg. That can't be None, so this sets it to some value.
My understanding is that this can only be None for a bare TreeOfBoxes (which doesn't have the particle_id_dtype), so not sure if this is a reasonable default? Does this even work in that case?
There was a problem hiding this comment.
I don't think it's OK to default this at all. I've removed the defaulting logic, plus the sketchy getattr in the caller. We'll see what breaks.
There was a problem hiding this comment.
Apparently, nothing. All the better!
There was a problem hiding this comment.
@inducer Forgot to push this? I'm still seeing it in the merged version in main.
There was a problem hiding this comment.
Very weird. That force-push apparently went nowhere? Not sure. #114 has my changes.
|
Hm.. The |
| if isinstance(field_type, GenericAlias | _BaseGenericAlias | _SpecialForm): | ||
| # NOTE: anything except a Union is not an array |
There was a problem hiding this comment.
The issue was mainly here, because ObjectArray1D[Array] is actually a _BaseGenericAlias, so this would incorrectly return False.
It now just checks for special forms, but we can't use too much of the arraycontext implementation because some of the boxtree classes have tuple[Array, ...] and Literal, which aren't supported by all_type_leaves_satisfy_predicate.
alexfikl
force-pushed
the
type-traversal
branch
2 times, most recently
from
d97d417 to
e474a34
Compare
|
LGTM, thanks! |
2 participants
This fixes some actual errors (e.g.
from_sep_smaller_by_levelis anObjectArray1D) and adds some more types.