Skip to content

CLN: Add comment and clarify if condition in indexing #37960

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 19, 2020
Merged

CLN: Add comment and clarify if condition in indexing #37960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c84311
Implement review comments
phofl Nov 19, 2020
c084942
Change test names
phofl Nov 19, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pandas/core/indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1821,7 +1821,7 @@ def _setitem_single_block(self, indexer, value, name: str):
return

indexer = maybe_convert_ix(*indexer)
if isinstance(value, ABCSeries) and name != "iloc" or isinstance(value, dict):
if (isinstance(value, ABCSeries) and name != "iloc") or isinstance(value, dict):
# TODO(EA): ExtensionBlock.setitem this causes issues with
# setting for extensionarrays that store dicts. Need to decide
# if it's worth supporting that.
Expand Down Expand Up @@ -1859,6 +1859,7 @@ def _setitem_with_indexer_missing(self, indexer, value):
if index.is_unique:
new_indexer = index.get_indexer([new_index[-1]])
if (new_indexer != -1).any():
# We get only in this method with loc, so can hard code
Copy link
Member

@jbrockmendel jbrockmendel Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in this method" -> "here"
Copy link
Member Author

@phofl phofl Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed,
return self._setitem_with_indexer(new_indexer, value, "loc")

# this preserves dtype of the value
Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/indexing/test_iloc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ def test_iloc_getitem_nonunique(self):
ser = Series([0, 1, 2], index=[0, 1, 0])
assert ser.iloc[2] == 2

def test_setitem_iloc_pure_position_based(self):
def test_iloc_setitem_pure_position_based(self):
Copy link
Member

@jbrockmendel jbrockmendel Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment about test naming applied to a few tests in the previous PR.

BTW i have a branch called "collect" in which i collect small edits that dont necessarily merit their own PRs, then periodically make a PR titled "assorted cleanups"
Copy link
Member Author

@phofl phofl Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah you are right :)

When I started contributing someone told me to try avoiding cluttering things together in one pr, which belong to different prs, so I tried to split everything up as good as possible.

Will collect clean ups in the future :)
Copy link
Member

@jbrockmendel jbrockmendel Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I started contributing someone told me to try avoiding cluttering things together in one pr, which belong to different prs, so I tried to split everything up as good as possible.

In general this is good advice. For the "assorted cleanups" PRs its important that the changes be pretty purely cosmetic so that they are super-easy to review.

Will collect clean ups in the future :)

It works for me but not for everyone. You find what works for you.
Copy link
Member Author

@phofl phofl Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help. Will try to collect them in the future if this is doable
# GH#22046
ser1 = Series([1, 2, 3])
ser2 = Series([4, 5, 6], index=[1, 0, 2])
Expand Down