BUG: Attributes are lost when subsetting columns in groupby

[rhshadrach]

• closes columns selection after groupby reset group_keys to True #9959
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Avoiding the behavior of Series here, e.g. df.groupby('a')['b'] as I think that will involve some API changes. Will follow up: ref #35443

[simonjayhawkins]

@rhshadrach test is failing on Linux py37_np_dev

=========================== short test summary info ============================
FAILED pandas/tests/groupby/test_groupby.py::test_subsetting_columns_keeps_attrs[squeeze-True]
=== 1 failed, 70023 passed, 4046 skipped, 1022 xfailed in 541.95s (0:09:01) ====

[pep8speaks]

Hello @rhshadrach! 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-08-31 20:16:14 UTC

[rhshadrach]

Thanks @simonjayhawkins, passing now.

[jreback]

does this also close: #35014 ?

[arw2019]

does this also close: #35014 ?

@jreback I ran the OP in 35014 on this branch, the error persists

[jreback]

@rhshadrach can you merge master and ping on green.

[rhshadrach]

@jreback master has been merged, failures are unrelated

[simonjayhawkins]

failures are unrelated

restarted

C:\Miniconda\envs\pandas-dev\lib\site-packages\hypothesis\core.py:637: in execute_once
    self.__flaky(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <hypothesis.core.StateForActualGivenExecution object at 0x00000234496FE130>
message = 'Hypothesis test_shift_across_dst(offset=<0 * MonthBegins>) produces unreliable results: Falsified on the first call but did not on a subsequent one'

    def __flaky(self, message):
        if len(self.falsifying_examples) <= 1:
>           raise Flaky(message)
E           hypothesis.errors.Flaky: Hypothesis test_shift_across_dst(offset=<0 * MonthBegins>) produces unreliable results: Falsified on the first call but did not on a subsequent one

C:\Miniconda\envs\pandas-dev\lib\site-packages\hypothesis\core.py:847: Flaky
--------------------------------- Hypothesis ----------------------------------
Falsifying example: test_shift_across_dst(
    offset=<0 * MonthBegins>,
)
Unreliable test timings! On an initial run, this test took 884.81ms, which exceeded the deadline of 500.00ms, but on a subsequent run it took 1.18 ms, which did not. If you expect this sort of variability in your test timings, consider turning deadlines off for this test by setting deadline=None.

You can reproduce this example by temporarily adding @reproduce_failure('5.24.3', b'AAAAAAA=') as a decorator on your test case

[jreback]

wait, what happend to the tests?

[rhshadrach]

@arw2019 @jreback: I added all attributes to the ndim=1 case except as_index and axis as these might be considered an API change. Failures are unrelated.

[jreback]

can you xfail these in the parameter list instead

[jreback]

also if you can add a test that replicates the original issue (unless your new ones cover)

[rhshadrach]

@jreback test is now xfail instead of skip; original issue is covered by the added test when attr="group_keys". Failure is unrelated (due to pyarrow).

[rhshadrach]

@jreback should have mentioned - if the parameters are labeled xfail, then we wouldn't be testing their behavior for DataFrameGroupBy (which I think we want to). So I've left it as an if statement after the frame case and before the series. Does that work?

[rhshadrach]

I've updated the tests with @arw2019's suggestion. To my surprise, the line

df.groupby('a', as_index=False)['b']

results in a DataFrameGroupBy rather than a SeriesGroupBy, and thus would successfully pass the test. I'm still of the opinion that this should instead raise (ref: #35443), but perhaps others disagree. I've marked it as xfail with strict=False, but perhaps it shouldn't be marked xfail at all? Any suggestions here are much appreciated.

Update: After rethinking, this test shouldn't be xfailed.

[jreback]

thanks @rhshadrach

good approach about separating something potentially controversial to another issue.