-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: BlockManager.get_slice require only slice arg #40262
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
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.
this is quite a lot of added code paths in index, are these really all tested?
@@ -1604,14 +1609,23 @@ def _blklocs(self): | |||
""" compat with BlockManager """ | |||
return None | |||
|
|||
def getitem_mgr(self, indexer) -> SingleBlockManager: |
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.
shouldn't this exist for BlockManager and just be no-op?
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.
its only ever used by Series
yes |
ok looks fine. we have sufficient asv's to cover? (assume we do) |
yes. this is motivated by the one discussed in joris's libreduction PR |
lgtm. can you rebase & ping on green. |
Ping |
non-slice objects are currently getting past mypy. This ensures we only get slices, will open up some optimization options.
Also did some docstring/typing cleanup in the neighborhood.
update: updated to implement Index._getitem_slice. Cuts about 10% off of the benchmark discussed in #40171 (comment) (as compared to current master, not then-master)