-
Notifications
You must be signed in to change notification settings - Fork 903
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 JIT groupby test suite #13813
Expand JIT groupby test suite #13813
Conversation
Marking this ready for review - I am going to work on solving some of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandon-b-miller Just to clarify, you're planning to merge this to introduce new xfailed tests, then fix them in subsequent PRs?
Correct, this is my plan, based on not knowing how long it might take to fix some of these. |
Co-authored-by: Bradley Dice <[email protected]>
/ok to test |
/ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small cleanup requests, otherwise LGTM!
np.random.seed(0) | ||
def groupby_jit_data_small(): | ||
""" | ||
Small dataset for testing groupby apply with jit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write this comment with complete sentences / capitalization? It also feels like it's wrapped pretty harshly, like 50-60 chars / line? Same request for other comments below.
/ok to test |
/ok to test |
/merge |
This PR reorganizes and expands the test suite for groupby apply functions using the JIT engine to include nan cases and cases where the groups are larger than a single thread block.