Skip to content

TST: Add test for groupby aggregatation with nested dictionary #50079

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 3 commits into from
Dec 8, 2022
Merged

TST: Add test for groupby aggregatation with nested dictionary #50079

merged 3 commits into from
Dec 8, 2022

Conversation

talukder-sowrov
Copy link
Contributor

MarcoGorelli
MarcoGorelli suggested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

test looks fine, can you run the pre-commit checks please? check the contributing guide for how

@MarcoGorelli MarcoGorelli added Needs Tests Unit test(s) needed to prevent regressions Groupby labels Dec 6, 2022
@rhshadrach rhshadrach added this to the 2.0 milestone Dec 6, 2022
@rhshadrach rhshadrach added the Apply Apply, Aggregate, Transform, Map label Dec 6, 2022
MarcoGorelli
MarcoGorelli approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me - @rhshadrach any comments?

rhshadrach
rhshadrach requested changes Dec 8, 2022
View reviewed changes
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 good - just a request on the test name.

pandas/tests/groupby/aggregate/test_aggregate.py Outdated
@@ -468,6 +468,16 @@ def test_groupby_agg_coercing_bools():
tm.assert_series_equal(result, expected)


def test_groupby_agg_nested_dictionary():
Copy link
Member

Choose a reason for hiding this comment

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

You're not testing nested dictionaries here, so I think this name is misleading. I think we test dictionary behavior elsewhere, what seems likely to be novel about this test is that you are using [["B"]]; this calls the function _gotitem in the groupby code. So I'd suggest a name like: test_groupby_agg_dict_with_gotitem

Copy link
Member

@rhshadrach rhshadrach Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

getitem (as in the dunder __getitem__) would also work - actually, the more I think about it, the more I'd prefer getitem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

rhshadrach
rhshadrach approved these changes Dec 8, 2022
View reviewed changes
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.

lgtm

@rhshadrach rhshadrach merged commit 14991df into pandas-dev:main Dec 8, 2022
@rhshadrach
Copy link
Member

Thanks @talukder-sowrov!

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

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

@rhshadrach rhshadrach rhshadrach approved these changes

Assignees
No one assigned
Labels
Apply Apply, Aggregate, Transform, Map Groupby Needs Tests Unit test(s) needed to prevent regressions
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

"SpecificationError: nested dictionary is ambiguous in aggregation" in a certain case of groupby-aggregation
3 participants
@talukder-sowrov @rhshadrach @MarcoGorelli