Skip to content

DOC: Fixed type hints for GroupBy.{any, all} #36284

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

Merged

Conversation

ytakashina
Copy link
Contributor

@ytakashina ytakashina commented Sep 11, 2020

The current documentation says the return value of GroupBy.all and GroupBy.any is bool.

    def any(self, skipna: bool = True):
        """
        Return True if any value in the group is truthful, else False.

        Parameters
        ----------
        skipna : bool, default True
            Flag to ignore nan values during truth testing.

        Returns
        -------
        bool
        """

However, the actual returned type is DataFrame or Series (with the same shape as GroupBy.sum()).

In [25]: df
Out[25]:
     0    1    2    3
0  1.0  0.0  0.0  0.0
1  0.0  1.0  0.0  0.0
2  0.0  0.0  1.0  0.0
3  0.0  0.0  0.0  1.0

In [26]: df.groupby(0).any()
Out[26]:
         1      2      3
0
0.0   True   True   True
1.0  False  False  False

In [27]: type(df.groupby(0).any())
Out[27]: pandas.core.frame.DataFrame

This PR fixes the wrong type hints for GroupBy.{any, all}.

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

The current documentation says the return value of GroupBy.all and GroupBy.any is `bool`.

```
    def any(self, skipna: bool = True):
        """
        Return True if any value in the group is truthful, else False.

        Parameters
        ----------
        skipna : bool, default True
            Flag to ignore nan values during truth testing.

        Returns
        -------
        bool
        """
```

However, the actual returned type is DataFrame or Series (the same shape as `GroupBy.sum()`).

```
In [25]: df
Out[25]:
     0    1    2    3
0  1.0  0.0  0.0  0.0
1  0.0  1.0  0.0  0.0
2  0.0  0.0  1.0  0.0
3  0.0  0.0  0.0  1.0

In [26]: df.groupby(0).any()
Out[26]:
         1      2      3
0
0.0   True   True   True
1.0  False  False  False

In [27]: type(df.groupby(0).any())
Out[27]: pandas.core.frame.DataFrame
```

This PR fixes the wrong type hints for `GroupBy.{any, all}`.
@phofl
Copy link
Member

phofl commented Sep 11, 2020

That is a bit short imho. Could you maybe write something like here ? https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.core.groupby.GroupBy.sum.html

@ytakashina
Copy link
Contributor Author

Okay. I wrote some drafts for GroupBy.all() documentation. Which one do you think better?

  1. Computed all of values within each group.
    • just replaced sum by all in GroupBy.sum documentation.
  2. Computed all() of values within each group.
    • added parentheses to make it clear that all represents somewhat function (that is, it's not a pronoun).
  3. DataFrame or Series of boolean values, which is True if all elements are True within each group, False otherwise.
    • wrote whole description.

If you have any alternative, plz tell me.

@pep8speaks
Copy link

pep8speaks commented Sep 14, 2020

Hello @ytakashina! 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 2020-10-09 00:30:11 UTC

@ytakashina
Copy link
Contributor Author

Edited the description for the return value of GroupBy.{any, all} after receiving @rhshadrach's suggestion.

@jbrockmendel
Copy link
Member

Seems like a clear improvement over the status quo. @rhshadrach are you happy here?

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Looks great - thanks @ytakashina

@rhshadrach rhshadrach added this to the 1.2 milestone Sep 23, 2020
@rhshadrach
Copy link
Member

@ytakashina Can you merge master? I think it will fix CI.

@ytakashina
Copy link
Contributor Author

@rhshadrach CI Fixed!

@rhshadrach rhshadrach merged commit 653f694 into pandas-dev:master Oct 9, 2020
@rhshadrach
Copy link
Member

Thanks @ytakashina!

@ytakashina ytakashina deleted the ytakashina-patch-type-hinting-groupby branch October 9, 2020 01:50
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants