8000 ENH Add array_api compatibility to `dcg_score` by EdAbati · Pull Request #29339 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

ENH Add array_api compatibility to dcg_score #29339

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

EdAbati
Copy link
Contributor
@EdAbati EdAbati commented Jun 23, 2024

Reference Issues/PRs

Towards #26024

What does this implement/fix? Explain your changes.

It makes the dcg_score implementation compatible and tested with the Array API.

Any other comments?

TODOs

  • zip doesn't seem to work with array_api_strict need to investigate (or find an alternative)
  • move _cumulative_sum to utils (and change name to _cumulative_sum1d because does always _ravel)
  • fix hardcoded float.64 with mps
  • fix ignore_ties (still not working with Array API strict)

Very happy to hear your feedback on how to improve this. (Mostly regarding the issue with Array API strict) I'm also a bit unsure if we have to split always numpy vs array-api in this case

cc @ogrisel @OmarManzoor

Copy link
github-actions bot commented Jun 23, 2024

✔️ Linting Passed

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

Generated for commit: 1c27d26. Link to the linter CI: here

@EdAbati EdAbati marked this pull request as ready for review July 23, 2024 21:28
Copy link
Contributor
@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @EdAbati

discount_cumsum = np.cumsum(discount)
cumulative_gains = [
_tie_averaged_dcg(y_t, y_s, discount_cumsum)
for y_t, y_s in zip(y_true, y_score)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't something like this work generically?

Suggested change
for y_t, y_s in zip(y_true, y_score)
for y_t, y_s in xp.stack((y_true, y_score), axis=1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems to make array_api_strict complain if y_true and y_score don't have the same type (like in this test case): TypeError: array_api_strict.int64 and array_api_strict.float64 cannot be type promoted together

(array_api_strict complained a lot while working on this 😅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0