Skip to content

MAINT Minor adjustments to array_api usage in PCA #26898

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

Closed
wants to merge 2 commits into from

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Follow up to #26315

What does this implement/fix? Explain your changes.

Small adjustments to use less branching and functionality from our _array_api module.

@thomasjpfan thomasjpfan added the Quick Review For PRs that are quick to review label Jul 25, 2023
@github-actions
Copy link

github-actions bot commented Jul 25, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: eadd239. Link to the linter CI: here

@@ -275,8 +275,7 @@ def randomized_range_finder(
# from float64 to float32 in asarray might not always be accepted as only
# casts following type promotion rules are guarateed to work.
# https://github.com/data-apis/array-api/issues/647
if is_array_api_compliant:
Q = xp.asarray(Q, device=device(A))
Q = xp.asarray(Q, device=device(A))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may still need the compliance check for use on sparse data (current test failures).

Suggested change
Q = xp.asarray(Q, device=device(A))
if is_array_api_compliant:
Q = xp.asarray(Q, device=device(A))

Comment on lines -866 to +865
device = getattr(u, "device", None)
u_device = device(u)
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly using device fails for dataframes. If device = getattr(u, "device", None) represent the semantics we want, we could modify the device function directly to do the same.

@thomasjpfan
Copy link
Member Author

I prefer not to abstract dataframe and sparse handling into the _array_api module. In this case, I think the current code on main looks fine as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:utils Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants