Skip to content

BUG: Add test to ensure, that bug will not occur again. #33058 #33072

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 4 commits into from
Apr 10, 2020
Merged

BUG: Add test to ensure, that bug will not occur again. #33058 #33072

merged 4 commits into from
Apr 10, 2020

Conversation

phofl
Copy link
Member

@phofl phofl commented Mar 27, 2020 •
edited
Loading

Should I add a whatsnew entry? It's not really new, if it is backported to 1.0.1.

@simonjayhawkins
Copy link
Member

Thanks @phofl for the PR. changing closes #33058 to xref until a decision re backport.

Should I add a whatsnew entry?

not needed

simonjayhawkins
simonjayhawkins approved these changes Mar 27, 2020
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins 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.

Thanks @phofl lgtm pending green

@simonjayhawkins simonjayhawkins added Groupby Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite labels Mar 27, 2020
@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Mar 27, 2020
@simonjayhawkins
Copy link
Member

marked as 1.1 but could backport if fixed on 1.0.x

@simonjayhawkins
Copy link
Member

@phofl can you merge upstream master

@phofl
Merge branch 'master' of https://github.com/pandas-dev/pandas into ad…
41e4374 
…d_test_33058

� Conflicts:
�	pandas/tests/groupby/test_apply.py
@pep8speaks
Copy link

pep8speaks commented Apr 2, 2020 •
edited
Loading

Hello @phofl! 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-04-02 19:12:29 UTC

@phofl
Copy link
Member Author

phofl commented Apr 2, 2020

@simonjayhawkins Done.

simonjayhawkins
simonjayhawkins approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@phofl lgtm @jorisvandenbossche

@WillAyd
Copy link
Member

WillAyd commented Apr 7, 2020

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

WillAyd
WillAyd requested changes Apr 10, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

So I'm not sure this is worth adding a test for - mutating the argument during a .apply seems fraught with peril and I'm a little hesitant to make guarantees about behavior

@jorisvandenbossche do you have a strong preference towards adding this?

@jreback
Copy link
Contributor

jreback commented Apr 10, 2020

So I'm not sure this is worth adding a test for - mutating the argument during a .apply seems fraught with peril and I'm a little hesitant to make guarantees about behavior

@jorisvandenbossche do you have a strong preference towards adding this?

this is fine, we currently support mutating inside an apply, though we have an issue to deprecate.

@WillAyd
Copy link
Member

WillAyd commented Apr 10, 2020

sounds good

jreback
jreback requested changes Apr 10, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

see my comments & ping on green.

pandas/tests/groupby/test_apply.py
@@ -880,3 +880,24 @@ def test_apply_function_index_return(function):
index=pd.Index([1, 2, 3], name="id"),
)
tm.assert_series_equal(result, expected)


Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this in a new file, call it test_apply_mutate.py; there are 2 tests

pandas/tests/groupby/test_groupby.py:def test_mutate_groups():
pandas/tests/groupby/test_groupby.py:def test_no_mutate_but_looks_like():

that should also be moved there.

@WillAyd WillAyd merged commit 31ea45f into pandas-dev:master Apr 10, 2020
@WillAyd
Copy link
Member

WillAyd commented Apr 10, 2020

Thanks @phofl

@WillAyd
Copy link
Member

WillAyd commented Apr 10, 2020

Sorry @jreback merged while you were commented; i can do in a follow up

@WillAyd WillAyd mentioned this pull request Apr 10, 2020
Merged
@jreback
Copy link
Contributor

jreback commented Apr 10, 2020

thanks @WillAyd

@simonjayhawkins simonjayhawkins mentioned this pull request Apr 17, 2020
Closed
@phofl phofl deleted the test_for_33058 branch June 14, 2020 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd requested changes

@jreback jreback jreback requested changes

@simonjayhawkins simonjayhawkins simonjayhawkins approved these changes

Assignees
No one assigned
Labels
Groupby Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
5 participants
@phofl @simonjayhawkins @pep8speaks @WillAyd @jreback