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 ReportBase: 61.77% // Head: 61.80% // Increases project coverage by
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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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