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

Closes #2559 - dataframe_test.py conversion for new framework #2580

Merged

Conversation

Ethan-DeBandi99
Copy link
Contributor

Closes #2559

Updates the existing dataframe_test.py to function within the new test framework. Specific test for to_pandas was removed because this functionality is used in the majority of tests and does not require testing on its own. There is not a lot of parameterization here as I believe the cases we are dealing with are fairly representative. We may want to add additional general case testing in the future, but should not be required here.

This PR also adds a to_list method to ak.Series for use within test validation.

This PR updates ak.array to check the first element of an ndarray if its dtype=numpy.object_. This case can also occur with BigInt so we need to determine if the elements are strings or not for processing. This required a simple modification to an if block. I validated that all existing test cases pass with this modification as well.

Copy link
Contributor

@jaketrookman jaketrookman 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

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Only requesting changes for updating the test_drop since that's currently not testing the right thing for the inplace case. The rest is not as important

PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
PROTO_tests/tests/dataframe_test.py Outdated Show resolved Hide resolved
@Ethan-DeBandi99
Copy link
Contributor Author

@pierce314159 - I believe I addressed everything, but please let me know if I missed something.

@Ethan-DeBandi99 Ethan-DeBandi99 force-pushed the 2559_dataframe_test_upd branch from 61d513f to 169f0d0 Compare July 20, 2023 12:03
Copy link
Member

@stress-tess stress-tess 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!

@stress-tess stress-tess enabled auto-merge July 20, 2023 17:13
@stress-tess stress-tess added this pull request to the merge queue Jul 20, 2023
Merged via the queue into Bears-R-Us:master with commit 78e0a7f Jul 20, 2023
@Ethan-DeBandi99 Ethan-DeBandi99 deleted the 2559_dataframe_test_upd branch July 20, 2023 19:24
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.

dataframe_test.py Conversion for new test framework
4 participants