nio-blocks / counter

Counter, CounterFast, and NumericCounter count the number of signals that pass through the block
0 stars 1 forks source link

Remove reset groups from _cumulative_count #37

Closed shadetree01010100 closed 5 years ago

shadetree01010100 commented 5 years ago

QUESTION: Should resetting a counter also remove the group from _groups? I think that it should, unless I'm missing something: after sending a reset command, I expect that those group(s) should not appear in the response to a groups command.

EDIT: reset does not take a group param, so in fact the questions is "should _groups be cleared on reset()?"

UPDATE: e2515b4 implements this.

mattdodge commented 5 years ago

This is some serious deja vu from way back in the day. This has always kind of been a tricky one though. On one hand, if you clear out groups on reset you never really know if a group has 0 signals. Sometimes that is useful information (I'm reminded of way a long time ago when we used these blocks to compare volumes on different social media platforms and Google Plus would be at 0 a lot). On the other hand, if you don't clear them on reset when do you ever clear them? The list of "possible groups" goes on forever and ever.

I think the approach we take more often (and also the approach I think I'm in favor of) across all of the blocks though is to just let the groups grow and not clear them out. This also allows us to command the block to see the groups that have ever been processed by the block as well.

If we wanted to add this behavior I would probably suggest that it should be hidden behind an advanced boolean property at the very least.

shadetree01010100 commented 5 years ago

Great feedback, thank you. I reverted that commit so this PR will now remove group keys from _cumulative_count in reset(). This only affects the behavior of the TallyCounter block, so that only groups that have been processed since reset are included inside tally

shadetree01010100 commented 5 years ago

The more I think about it the more I think that this should be an advanced property.