New accumulator semantics #472
Conversation
Keeping the old accumulators intact but allowing new ones to be used in processor output
They need to stay as AccumulatorABC for backwards compatibility in user code for now.
coffea/processor/accumulator.py
Outdated
| elif isinstance(a, MutableMapping) and isinstance(b, MutableMapping): | ||
| out = copy.copy(a) | ||
| out.clear() | ||
| for key in set(a) & set(b): |
There was a problem hiding this comment.
Not sure if it matters that much but minimizing the number of times we iterate over keys of a and b could be important for large mappings. The set operations are all very fast but the creation of set(a) and set(b) may not be.
a_keyset = set(a)
b_keyset = set(b)
for key in a_keyset & b_keyset:
...If it's possible to non-hideously get rid of the walk over a from the instantiation of out, that would be good too.
coffea/processor/accumulator.py
Outdated
| if isinstance(a, MutableSet) and isinstance(b, MutableSet): | ||
| return operator.ior(a, b) | ||
| elif isinstance(a, MutableMapping) and isinstance(b, MutableMapping): | ||
| for key in set(a) & set(b): |
There was a problem hiding this comment.
same comment as for add about re-instantiating sets that you use over and over.
| out[key] = copy.deepcopy(a[key]) | ||
| for key in set(b) - set(a): | ||
| out[key] = copy.deepcopy(b[key]) | ||
| return out |
There was a problem hiding this comment.
It makes more sense in iadd, but do we want add to always take the type of the first operand? I think the usual behavior would be to take the first common non-abstract type shared by a and b (counting back from the passed types)?
I'm fine with this being the rule so long as we are clear about it.
What about user defined accumulables which are somehow not compatible with each other? We should detect that and throw an error, to keep people from getting confusing answers.
There was a problem hiding this comment.
The eventual call to a + b would throw if they are not compatible
There was a problem hiding this comment.
There is a loss of the type of the mapping b here but since I figure these are almost always just dictionaries no need to work too hard to resolve the common type of a and b. It would be a challenging problem. I had originally thought I would convert everything to plain dict in the output, but it might be nice for user to have some chance of receiving what they put in (e.g. defaultdict)
There was a problem hiding this comment.
Yeah for sure the common case is covered well by this. I'm only concerned with not-to-farfetched uncommon cases from people who like tinkering.
It's also fine to simply say as a rule that for different input types the lhs operand's type is taken. That gives enough control to the user and just requires docs/comments.
There was a problem hiding this comment.
Another option is to require type(a) == type(b) which in this context is how we would expect things to be used regardless. For some of our executors you can't really guarantee what ends up on lhs in a reproducible way since it depends on the order the jobs finish
There was a problem hiding this comment.
I went for a bit of a softer type check, take a look at the tests to see how it plays out
There was a problem hiding this comment.
That's also a fine solution. I think what we have right now is not confusing.
Keeping the old accumulators intact but allowing new ones to be used in processor output