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.

Sign up for GitHub

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

Jump to bottom

BUG: isclose: fix multidevice for equal_nan=True #177

Merged
merged 6 commits into from
Mar 25, 2025
Merged

BUG: isclose: fix multidevice for equal_nan=True #177

merged 6 commits into from
Mar 25, 2025

Conversation

lithomas1
Copy link
Contributor

xp.asarray(True) was missing a device argument, which causes array-api-strict with a device argument to fail in where.

Since as of array api 2024.12 we can pass in scalars directly to where, I've replaced asarray(True) with just True.

lithomas1
lithomas1 commented Mar 25, 2025
View reviewed changes
@lithomas1
Copy link
Contributor Author

@lucascolley
Do you mind taking a quick look at this PR?

lucascolley
lucascolley reviewed Mar 25, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

we already have

array-api-extra/tests/conftest.py

Lines 148 to 161 in acbdf72

@pytest.fixture
def device(
library: Backend, xp: ModuleType
) -> Device: # numpydoc ignore=PR01,RT01,RT03
"""
Return a valid device for the backend.
Where possible, return a device that is not the default one.
"""
if library == Backend.ARRAY_API_STRICT:
d = xp.Device("device1")
assert get_device(xp.empty(0)) != d
return d
return get_device(xp.empty(0))

it looks like we just forgot to add a test_device for isclose, see e.g.

array-api-extra/tests/test_funcs.py

Lines 435 to 437 in acbdf72

def test_device(self, xp: ModuleType, device: Device):
x = xp.asarray([1, 2, 3], device=device)
assert get_device(cov(x)) == device

@lithomas1 lithomas1 force-pushed the fix-isclose-multidevice branch 2 times, most recently from 757996d to b1e6639 Compare March 25, 2025 14:34
@lithomas1
Copy link
Contributor Author

Wow, totally missed that!

Thanks, that's a lot cleaner.

@lithomas1 lithomas1 requested a review from lucascolley March 25, 2025 14:36
@lithomas1 lithomas1 mentioned this pull request Mar 25, 2025
Open
lucascolley
lucascolley approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

thanks Thomas!

@lucascolley lucascolley added the bug Something isn't working label Mar 25, 2025
@lucascolley lucascolley changed the title BUG: Fix isclose multidevice for equal_nan=True BUG: isclose: fix multidevice for equal_nan=True Mar 25, 2025
@lucascolley lucascolley added this to the 0.7.1 milestone Mar 25, 2025
@lucascolley
Copy link
Member

looks like there is a pre-commit failure

@lithomas1
Copy link
Contributor Author

lithomas1 commented Mar 25, 2025 •
edited
Loading

Sorry, sent in a fix!
Can you kick off CI again for me?

@lucascolley
Copy link
Member

Unfortunately the xp_assert infra is not ready for array-api-strict arrays which can't be converted to NumPy arrays.

I'd be happy for you to modify the test to be just a device check with get_device. Alternatively, you could look into making the xp_assert infra work, but I'm not sure what the best way to go about doing that is.

@lithomas1 lithomas1 force-pushed the fix-isclose-multidevice branch from fc26a74 to bdc41bd Compare March 25, 2025 17:04
@lithomas1
Copy link
Contributor Author

I'd be happy for you to modify the test to be just a device check with get_device. Alternatively, you could look into making the xp_assert infra work, but I'm not sure what the best way to go about doing that is.

I added a conversion to the CPU device in array-api-strict which is what scikit-learn does as well I think.
Let me know if that approach works for you.

lucascolley
lucascolley reviewed Mar 25, 2025
View reviewed changes
src/array_api_extra/_lib/_testing.py Outdated
Comment on lines 109 to 113
if is_array_api_strict_namespace(xp):
# __array__ doesn't work on array-api-strict device arrays
# We need to convert to the CPU device first
actual = np.asarray(xp.asarray(actual, device=xp.Device("CPU_DEVICE")))
desired = np.asarray(xp.asarray(actual, device=xp.Device("CPU_DEVICE")))
Copy link
Member

Choose a reason for hiding this comment

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

ah, yes, this is a good call, thanks!

@lucascolley lucascolley force-pushed the fix-isclose-multidevice branch from bdc41bd to 1236e3a Compare March 25, 2025 17:40
@lucascolley lucascolley merged commit f55076f into data-apis:main Mar 25, 2025
11 checks passed
@lithomas1
Copy link
Contributor Author

Thanks for the review and for fixing typing!

@lithomas1 lithomas1 deleted the fix-isclose-multidevice branch March 25, 2025 19:10
NeilGirdhar pushed a commit to NeilGirdhar/array-api-extra that referenced this pull request Apr 2, 2025
@lithomas1 @lucascolley @NeilGirdhar
BUG: isclose: fix multidevice for equal_nan=True (data-apis#177)
370eee0 
* BUG: Fix isclose multidevice

* test the right way

* fix pre-commit

* convert to CPU in xp_assert_equal

* fixes

* fix tests

---------

Co-authored-by: Lucas Colley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucascolley lucascolley lucascolley approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
0.7.1
Development

Successfully merging this pull request may close these issues.
2 participants
@lithomas1 @lucascolley