-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: assorted, mostly typing #29419
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
CLN: assorted, mostly typing #29419
Conversation
@@ -971,7 +972,7 @@ def _cython_agg_blocks(self, how, alt=None, numeric_only=True, min_count=-1): | |||
locs = block.mgr_locs.as_array | |||
try: | |||
result, _ = self.grouper.aggregate( | |||
block.values, how, axis=agg_axis, min_count=min_count | |||
block.values, how, axis=1, min_count=min_count |
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.
Interesting that we had essentially hardcoded this value all this time...
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.
I think all of these changes look okay, but I also feel that this PR covers a little too much ground and could have benefited from being broken up into smaller parts (e.g. typing, Cython refactoring).
I somewhat on the fence, so would be good to get some other eyes to look at this.
Totally reasonable. If others find this a deal-breaker I'll split this up. Otherwise I'll apply this advice in future CLN PRs |
this is ok, but generally yes would like to split largish changes. note to @WillAyd this might conflict with your groupby PR? |
diff_2d no longer needs to be in the pxi.in file, so moved it to the pyx
A couple of recently-identified bugs in the groupby code are caused by passing incorrect types, so im getting more motivated to add annotations in/around the affected code.