-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Implement BlockManager.iset #32350
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
Implement BlockManager.iset #32350
Conversation
@@ -1087,7 +1087,7 @@ def value_getitem(placement): | |||
unfit_mgr_locs = np.concatenate(unfit_mgr_locs) | |||
unfit_count = len(unfit_mgr_locs) | |||
|
|||
new_blocks = [] | |||
new_blocks: List[Block] = [] |
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.
rather than add the variable annotation, does new_blocks need to be initialized outside the if else? make_block has the correct return type and the extend wouldn't be needed. (but I guess this mypy fixup is orthogonal to this PR)
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'm not a fan of variable annotations when not required. but not a blocker.
pandas/core/internals/managers.py
Outdated
def set(self, item: Label, value): | ||
""" | ||
Set new item in-place. Does not consolidate. Adds new Block if not | ||
contained in the current set of items |
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.
minor nit. can you aim for one-liners for PEP 257compliance
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.
lgtm. @simonjayhawkins comments & merge on green.
Thanks @jbrockmendel |
Preliminary to the PR that fixes
setitem_with_indexer
(#22036, #15686)