Skip to content

DOC/TST: Clarify Series.str.get supports passing hashable label #47918

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

Jump to bottom
Merged
merged 7 commits into from
Aug 15, 2022
Merged

DOC/TST: Clarify Series.str.get supports passing hashable label #47918

merged 7 commits into from
Aug 15, 2022

Conversation

xr-chen
Copy link
Contributor

@xr-chen xr-chen commented Aug 1, 2022 •
edited
Loading

@BeRT2me
Copy link

BeRT2me commented Aug 1, 2022

NO! #47911 (comment)

phofl
phofl reviewed Aug 1, 2022
View reviewed changes
doc/source/whatsnew/v1.5.0.rst Outdated
@@ -279,6 +279,7 @@ Other enhancements
- :class:`Series` reducers (e.g. ``min``, ``max``, ``sum``, ``mean``) will now successfully operate when the dtype is numeric and ``numeric_only=True`` is provided; previously this would raise a ``NotImplementedError`` (:issue:`47500`)
- :meth:`RangeIndex.union` now can return a :class:`RangeIndex` instead of a :class:`Int64Index` if the resulting values are equally spaced (:issue:`47557`, :issue:`43885`)
- :meth:`DataFrame.compare` now accepts an argument ``result_names`` to allow the user to specify the result's names of both left and right DataFrame which are being compared. This is by default ``'self'`` and ``'other'`` (:issue:`44354`)
- :meth:`Series.str.get` now raises when ``i`` is not a integer (:issue:`47911`)
Copy link
Member

Choose a reason for hiding this comment

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

Could you specify the error?

phofl
phofl reviewed Aug 1, 2022
View reviewed changes
Copy link
Member

@phofl phofl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Small comment, otherwise Lgtm

i is positional, e.g. strings should raise

edit: pre commit is failing, you can check this locally

@BeRT2me
Copy link

BeRT2me commented Aug 1, 2022 •
edited
Loading

i is only positional in the documentation... it's definitely not strictly positional in current functionality.
https://github.com/pandas-dev/pandas/blob/14de3fd9ca4178bfce5dd681fa5d0925e057c04d/pandas/core/strings/object_array.py#L249:L257

@mroeschke
Copy link
Member

mroeschke commented Aug 1, 2022 •
edited
Loading

I am starting to agree with @BeRT2me, I don't think we can change this outright.

I think the fact that str.get works for object with a dict element series was just poorly described as there were past issues explaining some usage of this functionality. e.g. #20671

I would be +1 to keep supporting this (although there's cognizant dissonance with str.get working on dict elements). I think clarifying pos can be a hashable dict label and adding unit tests would be good instead.

@phofl
Copy link
Member

phofl commented Aug 1, 2022

Do you want to deprecate or add this to the documentation?

@BeRT2me
Copy link

BeRT2me commented Aug 1, 2022

How Series.str works for anything (list, tuple, dict, set, etc.) that isn't a string hasn't made much sense logically for a while.

In my opinion, mirroring how dict.get does things by allowing a default value to be specified if the key/index isn't found would be a positive change.

Not forgetting to update the documentation to match would be nice as well...

@xr-chen
Copy link
Contributor Author

xr-chen commented Aug 1, 2022 •
edited
Loading

I think if we have a list of dictionaries like

lst_of_dict = [
   {"name": "Hello", "value": "World"},
   {"name": "Goodbye", "value": "Planet"}
]

we could construct a DataFrame by pd.DataFrame(lst_of_dict), and DataFrame supports pass default value to DataFrame.get. I have no idea about whether we should deprecate pass non-integer type in Series.str.get or not.

@xr-chen
Copy link
Contributor Author

xr-chen commented Aug 2, 2022

Should _str_get supports default value argument as @BeRT2me suggested?

@xr-chen xr-chen changed the title ERR: Raise TypeError when Series.str.get passed a non-integer argument DOC/TST: Clarify Series.str.get supports passing hashable label Aug 3, 2022
@mroeschke
Copy link
Member

Should _str_get supports default value argument as @BeRT2me suggested?

I would save that for another issue to discuss

@mroeschke
Copy link
Member

Appears some doc checks are failing

@xr-chen
Copy link
Contributor Author

xr-chen commented Aug 8, 2022

Honestly, I don't know what's going wrong from the Traceback of the failed check.

@mroeschke
Copy link
Member

Might want to merge in the main branch into this PR. I believe there was some doc failures that were fixed recently.

@pep8speaks
Copy link

pep8speaks commented Aug 8, 2022 •
edited
Loading

Hello @xr-chen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-08-09 01:03:20 UTC

mroeschke
mroeschke reviewed Aug 8, 2022
View reviewed changes
pandas/tests/strings/test_strings.py Outdated
def test_get_with_dict_label():
# GH47911
s = Series(
[{"name": "Hello", "value": "World"}, {"name": "Goodbye", "value": "Planet"}]
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add an element where "name" does not exist in the dictionary?

@mroeschke mroeschke added Testing pandas testing functions or related to the test suite Docs labels Aug 8, 2022
@mroeschke mroeschke added this to the 1.5 milestone Aug 9, 2022
mroeschke
mroeschke approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM. @phofl if you have any comments, especially opinions on supporting this.

@mroeschke mroeschke merged commit d5b4b33 into pandas-dev:main Aug 15, 2022
@mroeschke
Copy link
Member

Thanks @xr-chen

@xr-chen xr-chen deleted the strget branch August 15, 2022 17:58
YYYasin19 pushed a commit to YYYasin19/pandas that referenced this pull request Aug 23, 2022
@xr-chen @YYYasin19
DOC/TST: Clarify Series.str.get supports passing hashable label (pand…
3cc5fa4 
…as-dev#47918)

* gh 47911

* pre-commit issue

* add test and fix doc

* modified comment

* pep 8

* add more elements
noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
@xr-chen @noatamir
DOC/TST: Clarify Series.str.get supports passing hashable label (pand…
c2b216e 
…as-dev#47918)

* gh 47911

* pre-commit issue

* add test and fix doc

* modified comment

* pep 8

* add more elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phofl phofl phofl left review comments

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Docs Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.

DOC/TST: pandas.Series.str.get with dict elements supports passing hashable label
5 participants
@xr-chen @BeRT2me @mroeschke @phofl @pep8speaks