-
Notifications
You must be signed in to change notification settings - Fork 129
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.
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
filter: Use intermediate columns for grouping #1070
filter: Use intermediate columns for grouping #1070
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
+ Coverage 61.77% 61.80% +0.03%
==========================================
Files 52 52
Lines 6316 6321 +5
Branches 1550 1551 +1
==========================================
+ Hits 3902 3907 +5
Misses 2141 2141
Partials 273 273 ☔ View full report in Codecov by Sentry. |
Grouping by day wouldn't be that terrible as a side effect 🙃 |
f2a6d71
to
3043718
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to remove the side effect of grouping by day. This is open enough to add grouping by day as an official group-by category in the future if requested.
I only have one comment on the internal prefix string.
Previously: 1. year/month/day columns were created on the DataFrame used for grouping. 2. month was overwritten with (year, month). This allowed year to be used for grouping, but also the same for day which was an unintended side effect. Instead of adding columns with those names, use names with a more distinct prefix that can be safely discarded before grouping happens.
8375de9
to
7eacd26
Compare
Description of proposed changes
Previously:
This allowed year to be used for grouping, but also the same for day which was an unintended side effect. Instead of adding columns with those names, use names with a more distinct prefix that can be safely discarded before grouping happens.
Related issue(s)
Fixes #1069
Testing
Checklist