Fix Fitness Aggregation #201
Conversation
| @@ -288,7 +288,7 @@ def get(self, request): | |||
|
|
|||
| class FitnessUsage(APIView): | |||
| def safe_add(self, a, b): | |||
| return a + b if a and b else (a if a else b) | |||
| return None if a is None and b is None else (a or 0) + (b or 0) | |||
There was a problem hiding this comment.
this is the only real change. everything else was just me refactoring.
i'm pretty sure the problem was when a or b was 0 and the other None. then safe_add didn't behave as expected and could return None (depending on order of a, b).
and then you get None when you call this twice so safe_add(None, 0) = 0, but then safe_add(0, None) = None.
backend/penndata/views.py
Outdated
| @@ -370,14 +370,14 @@ def get_usage(self, room, date, num_samples, group_by, field): | |||
| usage = self.get_usage_on_date(room, curr, field) # usage for curr | |||
| # incorporate usage safely considering None (no data) values | |||
| usage_aggs = [ | |||
| (self.safe_add(acc[0], val), acc[1] + (1 if val is not None else 0)) | |||
| for acc, val in zip(usage_aggs, usage) | |||
| (self.safe_add(sum, val), count + (val is not None)) | |||
There was a problem hiding this comment.
probably still cleaner to put 1 if val is not None else 0, equivalent code but makes more sense than adding a boolean
backend/penndata/views.py
Outdated
| @@ -370,14 +370,14 @@ def get_usage(self, room, date, num_samples, group_by, field): | |||
| usage = self.get_usage_on_date(room, curr, field) # usage for curr | |||
| # incorporate usage safely considering None (no data) values | |||
| usage_aggs = [ | |||
| (self.safe_add(acc[0], val), acc[1] + (1 if val is not None else 0)) | |||
| for acc, val in zip(usage_aggs, usage) | |||
| (self.safe_add(sum, val), count + (val is not None)) | |||
| ] | ||
| # update min and max date if any data was logged | ||
| if any(usage): | ||
| min_date = min(min_date, curr) | ||
| max_date = max(max_date, curr) | ||
| ret = [usage_agg[0] / usage_agg[1] if usage_agg[1] else None for usage_agg in usage_aggs] | ||
| ret = [(sum / count) if count else None for (sum, count) in usage_aggs] |
There was a problem hiding this comment.
I believe the crash was because of here when sum is None but count is not None. From the safe_add change, it looks like sum can still be None when count is not None.
Do we have any thoughts on how to address this?
There was a problem hiding this comment.
we can probably just modify the conditional to be if count and sum is not None
There was a problem hiding this comment.
if count checks that count is non-zero. Do you have an example of when sum is None but count is not 0?
The logic is that count can only be incremented when we safe_add a non-None value to sum, and safe_add will never be able to return None if we put in a non-None value (unlike before, hence why the problem before)..
My thinking is count = 0 <=> sum is None, or at least the logic wants it to be that way, so I'd rather not check both and get an error in the case this doesn't happen so we know the logic didn't check out, rather than silently sweeping it under the rug (like if we had checked for both before, I wouldn't have known safe_add is broken)
There was a problem hiding this comment.
Ah I see. Okay that makes sense. thanks!
| ] | ||
| # update min and max date if any data was logged | ||
| if any(usage): | ||
| min_date = min(min_date, curr) | ||
| max_date = max(max_date, curr) | ||
| ret = [usage_agg[0] / usage_agg[1] if usage_agg[1] else None for usage_agg in usage_aggs] | ||
| ret = [(sum / count) if count else None for (sum, count) in usage_aggs] |
There was a problem hiding this comment.
Ah I see. Okay that makes sense. thanks!
Fixing: https://penn-labs.slack.com/archives/C05Q2MEGY3T/p1694365151920569