Skip to content

Fix codecov in tests for array api in pairwise metrics #29036

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

Merged
merged 1 commit into from
May 17, 2024

Conversation

OmarManzoor
Copy link
Contributor

Reference Issues/PRs

Follow up of #29014

What does this implement/fix? Explain your changes.

Tried adding the condition which codecov reported as partially checked.

Any other comments?

CC: @ogrisel Do we need to add this or can we ignore?

Copy link

✔️ Linting Passed

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

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

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I think it might help catch weird bugs, so let's test this case.

@betatim betatim merged commit 48669a5 into scikit-learn:main May 17, 2024
33 of 34 checks passed
Comment on lines +1900 to +1902
check_array_api_metric(
metric, array_namespace, device, dtype_name, a_np=X_np, b_np=Y_np
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi all, just realised that we are not passing the metric_kwargs to the function (also in the function call below). I think it should be:

check_array_api_metric(
             metric, array_namespace, device, dtype_name, a_np=X_np, b_np=Y_np, ** metric_kwargs
         )

should I make another PR?

Copy link
Member

Choose a reason for hiding this comment

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

Ahrg! Yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize for missing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also missed it in the previous PR 😇

See fix here: #29045

@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
14 tasks
@OmarManzoor OmarManzoor deleted the fix_codecov branch May 20, 2024 11:34
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
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.

4 participants