FIX sklearn sample_weights checks for cupy and numpy<2 - #102
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the device-matching logic for sample weights in himalaya/kernel_ridge/_sklearn_api.py by checking if the dual coefficients are on the GPU and moving the weights to the CPU if not. The reviewer suggests a cleaner, branchless approach using getattr(self.dual_coef_, "device", "cpu") with backend.asarray to preserve explicit device-matching behavior across multi-GPU setups and maintain compatibility with older NumPy versions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if not backend.is_in_gpu(self.dual_coef_): | ||
| sw = backend.to_cpu(sw) |
There was a problem hiding this comment.
Instead of checking is_in_gpu and calling to_cpu, we can use getattr(self.dual_coef_, "device", "cpu") to get the device of self.dual_coef_ (defaulting to "cpu" for NumPy < 2 which lacks the .device attribute). This is cleaner, avoids branching, and preserves the explicit device-matching behavior of backend.asarray on multi-GPU setups.
| if not backend.is_in_gpu(self.dual_coef_): | |
| sw = backend.to_cpu(sw) | |
| device = getattr(self.dual_coef_, "device", "cpu") | |
| sw = backend.asarray(sw, device=device) |
There was a problem hiding this comment.
Since this only ever moves to the CPU, and there are no device IDs for 'cpu', I think the current check is actually safer.
| if not backend.is_in_gpu(self.dual_coef_): | ||
| sw = backend.to_cpu(sw) |
There was a problem hiding this comment.
Instead of checking is_in_gpu and calling to_cpu, we can use getattr(self.dual_coef_, "device", "cpu") to get the device of self.dual_coef_ (defaulting to "cpu" for NumPy < 2 which lacks the .device attribute). This is cleaner, avoids branching, and preserves the explicit device-matching behavior of backend.asarray on multi-GPU setups.
| if not backend.is_in_gpu(self.dual_coef_): | |
| sw = backend.to_cpu(sw) | |
| device = getattr(self.dual_coef_, "device", "cpu") | |
| sw = backend.asarray(sw, device=device) |
There was a problem hiding this comment.
See previous comment.
This PR fixes the following failing tests:
that had this traceback:
The device moving logic fails since
np.ndarraydoesn't support.devicebelow version 2. This logic was introduced with the MPS backend, so we should probably test that backend before merging.This was not an issue for
numpy>2.