-
Notifications
You must be signed in to change notification settings - Fork 33
Last dask fixes #110
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
Last dask fixes #110
Conversation
Co-Authored-By: Isaac Virshup <[email protected]>
@@ -44,7 +52,7 @@ | |||
def svd(x: Array, full_matrices: bool = True, **kwargs) -> SVDResult: | |||
if full_matrices: | |||
raise ValueError("full_matrics=True is not supported by dask.") | |||
return da.linalg.svd(x, **kwargs) | |||
return da.linalg.svd(x, coerce_signs=False, **kwargs) |
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.
Needed sign dask will do a svd_flip
on the data otherwise, which will make signs differ from numpy/scipy.
obj = np.asarray(obj).copy() | ||
#print(obj) | ||
return xp.array(obj, dtype=dtype, **copy_kwargs) | ||
return xp.array(obj, copy=True, dtype=dtype) |
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 think I reverted back to the original changes here (before my dask PR), but it'd be good to make sure.
cc @asmeurer (This should be ready, but I'd like to test on what I have with scikit-learn again, to catch any bugs) |
I've verified that the fixes here don't break scikit-learn. |
ping @asmeurer |
@@ -37,6 +37,9 @@ array_api_tests/test_data_type_functions.py::test_finfo[float32] | |||
# (I think the test is not forcing the op to be computed?) | |||
array_api_tests/test_creation_functions.py::test_linspace | |||
|
|||
# out.shape=(2,) but should be (1,) |
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.
Is there an upstream issue for this?
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.
Not yet - but I'll followup with dask
(I haven't been able to get this down to a MRE yet)
No description provided.