Conversation
majosm
force-pushed
the
empty-subcontainers
branch
from
974bdfd to
0ff5f70
Compare
majosm
force-pushed
the
empty-subcontainers
branch
2 times, most recently
from
4ab1b0b to
8bd3252
Compare
|
This has picked up some merge conflicts. Could you take a look? Also, ready to un-draft? |
majosm
force-pushed
the
empty-subcontainers
branch
from
8bd3252 to
cb8f3e7
Compare
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Thanks! LGTM aside from the all-empty handling, which I think ought to be quick to resolve.
| if result is None: | ||
| raise ValueError("cannot reduce empty array container") |
There was a problem hiding this comment.
Personally, I think reducing over an empty container is well-defined: just return the neutral element. reduce_func should be trusted to do the right thing. What was the reasoning for putting in this error message? Basically, what I'm pushing for is to get rid of it and return reduce_func([]) instead.
There was a problem hiding this comment.
If reduce_func worked for an empty list, I think this PR wouldn't be needed, right?
The motivation for this was for things like actx.np.max where reduce_func doesn't (currently) behave nicely for empty inputs. Should that maybe be fixed instead?
There was a problem hiding this comment.
For reference, calling np.max on an empty list raises
>>> np.max([])
ValueError: zero-size array to reduction operation maximum which has no identity
and requires specifying an initial value with np.max([], initial=np.inf). We should probably do something like that in actx.np too.
There was a problem hiding this comment.
The motivation for this was for things like
actx.np.maxwherereduce_funcdoesn't (currently) behave nicely for empty inputs.
That's a great point. I had not thought far enough to realize that, but I agree now.
Should that maybe be fixed instead?
I think that's exactly what we should do.
We should probably do something like that in
actx.nptoo.
I agree. While I wasn't loving numpy's interface of "no implicit neutral" element at first, I think it makes sense from the perspective of integers. I think we should aim to replicate this, both at the level of actx.np, and for the array container reductions under discussion here.
One subtlety is the ValueError for empty arrays. For symbolically-shaped pytato arrays, it may not be possible to do a perfect job.
There was a problem hiding this comment.
Did some work towards that:
- Introduce
ReductionOperationclass, accept 'initial' in reductions pytato#238 - Match numpy interface for empty reductions #136
That latter PR needs some more work done to it. @majosm, could you take a look?
| if result is None: | ||
| raise ValueError("cannot reduce empty array container") |
3 participants
Allows some (but not all) of the sub-containers in an array container to be empty in reductions. This allows reductions to be used on single-species CV objects in mirgecom.
Depends on #128 (not in an essential way, just avoiding dealing with merge conflicts).