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

Add scheduler option to assert_eq utilities #8610

Merged
merged 4 commits into from
Feb 9, 2022

Conversation

xinrong-meng
Copy link
Contributor

@xinrong-meng xinrong-meng commented Jan 23, 2022

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions github-actions bot added the array label Jan 23, 2022
@xinrong-meng
Copy link
Contributor Author

xinrong-meng commented Jan 23, 2022

Are test failures below infra-related?

@jsignell
Copy link
Member

Yeah the test failures mostly have to do with dask not being quite caught up with the latest release of pandas: #8580

@jsignell jsignell self-requested a review January 24, 2022 22:23
@jsignell
Copy link
Member

ok to test

@xinrong-meng
Copy link
Contributor Author

Thanks @jsignell !

Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This looks great! I just have one small suggestion

Comment on lines 114 to 117
if isinstance(a, Array) and isinstance(b, Array):
assert counter == 2
else:
assert counter == 0
Copy link
Member

Choose a reason for hiding this comment

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

I really like this test! But this part took me a minute to understand. I am wondering if something like this is clearer:

Suggested change
if isinstance(a, Array) and isinstance(b, Array):
assert counter == 2
else:
assert counter == 0
n_da_arrays = len([x for x in [a, b] if isinstance(x, Array)])
assert counter == n_da_arrays

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's so smart! Committed, thank you.

xinrong-meng and others added 2 commits February 9, 2022 10:05
Co-authored-by: Julia Signell <jsignell@gmail.com>
@jsignell jsignell added enhancement Improve existing functionality or make things work better tests Unit tests and/or continuous integration labels Feb 9, 2022
@jsignell jsignell merged commit fe8cff8 into dask:main Feb 9, 2022
@jsignell
Copy link
Member

jsignell commented Feb 9, 2022

Thanks @xinrong-databricks! This is in :)

@xinrong-meng
Copy link
Contributor Author

Thank you @jsignell !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array enhancement Improve existing functionality or make things work better tests Unit tests and/or continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add scheduler option to assert_eq utilities
3 participants