Skip to content
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

Expand LuxGroupby Tests and add bug fixes #287

Merged
merged 20 commits into from
Mar 2, 2021

Conversation

westernguy2
Copy link
Contributor

Overview

This pull request expands the tests for LuxGroupby to test if a LuxDataFrame is pre_aggregated after a groupby. It also patches the edge case of LuxDataFrame having a "name" column among other bug fixes.

Changes

This pull request is mainly for adding tests to LuxGroupby that didn't check if a LuxDataFrame is pre_aggregated after a groupby in certain cases.

In addition, this PR adds the "name" column back to the metadata, which patches the bug described in the above Overview. A test has been added to ensure this doesn't change.

Finally, this PR also patches small bugs relating to groupby and LuxGroupby. For instance, it adds apply as one of the methods extended from Pandas Groupby. It also fixes an edge case where the name of a Series was being changed. This is patched by directly editing unnamed columns (named by default as 0) to a blank string.

Example Output

The expected behavior should model the expected behavior of Pandas for the different patches described above.

Copy link
Contributor

@thyneb19 thyneb19 left a comment

Choose a reason for hiding this comment

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

The changes look good! Just added a comment on hos the test_name_column test could be adjusted. Thanks Kunal!


def test_name_column(global_var):
df = pd.read_csv("lux/data/car.csv")
new_df = df.rename(columns={"Name": "name"})
Copy link
Contributor

Choose a reason for hiding this comment

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

for this test, can we do a quick check that the values of the "name" column have not all been converted to None values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I added a few more assert statements to check for this case!

@thyneb19 thyneb19 requested a review from dorisjlee March 1, 2021 16:50
@dorisjlee
Copy link
Member

These changes looks great, thanks @westernguy2 ! (and thanks to @thyneb19 for helping with the review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants