Let AggregateExpFragments take callables as well as exp fragments #425
Conversation
| self.setattr_result("sum", FloatChannel) | ||
|
|
||
| def push_sum(): | ||
| self.sum.push(self.a.value.get() + 1 + self.b.value.get() + 1) |
There was a problem hiding this comment.
I would love this to be self.sum.push(self.a.result.get_last() + self.b.result.get_last()) but:
- currently the
get_lastis deliberately kernel only - we could add a sink-based interface that works by absorbing results...but then that's host-only so we can't run the test in a portable fashion
- I also considered having
AddOneFragmentstore alast_valueattribute and store the result there as well, but that also seems like a pretty miserable hack. We're essentially saying to users "getting the last value pushed to a result is a legitimate thing to do, but we won't support it so you have to manually duplicate result channels and handle all the accompanying synchronization issues yourself"
There was a problem hiding this comment.
We're essentially saying to users "getting the last value pushed to a result is a legitimate thing to do, but we won't support it so you have to manually duplicate result channels and handle all the accompanying synchronization issues yourself"
This is still preferrable to adding a half-baked implementation with unclear semantics to ndscan. "Magic" solutions that work only most of the time with unclear semantics (i.e. leaky abstractions) are much worse than just requiring users to write some straightforward Python code.
dnadlinger
force-pushed
the
tph/aggregate
branch
from
ce2268c to
5b1649c
Compare
|
Thanks, merged with a bug in the docstring (override_param() return value order) fixed. As for a test case, I'd say the dozen lines it takes to just add a separate Could you follow this up with such a straightforward smoke test, perhaps echoing the example from the docstring? |
Closes #416
See #416 for discussion
I haven't added a kernel test for this but would be happy to do so if requested. Not sure the best way of doing this - perhaps making all of
AddOneFragment's methods protable and then havingAddOneAggregatetake a build-time boolean flag to determine whether it runs on the kernel or host? Suggestions welcome!