Conversation
TimCookCountyDS
requested review from
jeancochrane and
wrridgeway
as code owners
jeancochrane
left a comment
jeancochrane
left a comment
There was a problem hiding this comment.
Thanks for the start here! Assuming this is all good, it's nice that this change is so simple. I'd like @wrridgeway to take a look at this too, since I suspect he has more experience with the metrics code than I do.
One high-level request: Can you flesh out the PR title so that it's more descriptive of the change? This is important because the PR title will become the title of the commit when we squash and merge this PR.
Also, I'm curious to get @wrridgeway's thoughts on the issue of null values that you raised in #458. It seems pretty strange to me, though I can't think of why your changes might introduce nulls.
| sum_fns_list <- list( | ||
| min = \(x) min(x, na.rm = TRUE), | ||
| q25 = \(x) quantile(x, na.rm = TRUE, probs = 0.25), | ||
| mean = \(x) mean(x, na.rm = TRUE), |
There was a problem hiding this comment.
[Question, non-blocking] I haven't spent much time with this metrics code, so I may just be lacking context here, but can you explain why we only want to add mean/SD metrics to the sum_fns_list and sum_sqft_fns_list, and not ys_fns_list or yoy_fns_list?
2 participants
Added mean and standard deviation to the gen_agg_stats function. This should close out issue #458 . See the comments I added to #458 for sanity checks in athena. (Writing those sanity checks really makes me think about @jeancochrane 's thoughts on unit and integration tests ).