-
Notifications
You must be signed in to change notification settings - Fork 922
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
Update pylibcudf
testing utilities
#15772
Update pylibcudf
testing utilities
#15772
Conversation
@@ -24,28 +24,28 @@ def metadata_from_arrow_array( | |||
return metadata | |||
|
|||
|
|||
def assert_column_eq(plc_column: plc.Column, pa_array: pa.Array) -> None: | |||
def assert_column_eq(expect: pa.Array, got: plc.Column) -> None: |
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.
Do we want to model this on
cudf/python/cudf/cudf/testing/_utils.py
Line 116 in 6d5f965
def assert_eq(left, right, **kwargs): |
We may not always be comparing an “expected/actual” result. The assert_eq just uses left/right names. The order is perhaps better defined by the caller, and hint at that intended use case in a docstring?
Additionally, it’s not obvious that this function expects a PyArrow and a pylibcudf object. Should we normalize this to accept either argument as either type, more like assert_eq?
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.
I updated this to be more symmetric, although I think we're stuck with having to have at least one of the objects be a pyarrow array, even if we don't know which one, unless we want to make broader changes. This is due to needing the metadata from the pyarrow array to compare nested data.
/merge |
Cleans up some testing utilities for pylibcudf as suggested in #15418 (comment).