Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Fitness Aggregation #201

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Fix Fitness Aggregation #201

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f47197
fix fitness aggregation
vcai122 Sep 13, 2023
097d582
remove add boolean
vcai122 Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions backend/penndata/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def linear_interpolate(self, before_val, after_val, before_date, current_date, after_date):
return (
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably still cleaner to put 1 if val is not None else 0, equivalent code but makes more sense than adding a boolean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine : (

for (sum, count), val in zip(usage_aggs, usage)
]
# 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably just modify the conditional to be if count and sum is not None

Copy link
Contributor Author

@vcai122 vcai122 Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Okay that makes sense. thanks!

return ret, min_date, max_date

def get(self, request, room_id):
Expand Down
Loading