-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: GroupBy.size created by TimeGrouper raises AttributeError #7600
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
Conversation
@@ -1665,6 +1666,22 @@ def levels(self): | |||
def names(self): | |||
return [self.binlabels.name] | |||
|
|||
def size(self): |
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.
why are you not using value_counts
? this should be very similar (if not call irectly), BaseGrouper.size
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.
There are 2 reasons.
value_counts
cannot fulfill intermediate timestamps which is not included in the group key. For example, if theTimeGrouper
categorize[2011-01-01, 2011-01-02, 2011-01-04]
with daily frequency, the resulted group keys must be[2011-01-01, 2011-01-02, 2011-01-03, 2011-01-04]
BinGrouper
doesn't retain originalIndex
to be directly passed tovalue_counts
. To usevalue_counts
, it must be created using similar logic.
indices = self.indices
labels = []
for k, v in compat.iteritems(indices):
labels.extend([k] * len(v))
value_counts(labels)
ok can u add a bench for size in that case? |
OK, added benchmarks. |
can I post benchmarks for those 2? thxs |
Here it is. Cannot measure base performance of
|
run then with |
Yep, current perf of |
BUG: GroupBy.size created by TimeGrouper raises AttributeError
Related to #7453 2nd issue:
GroupBy.size
created byTimeGrouper
raisesAttributeError