Skip to content

MAINT Parameter validation for d2_absolute_error_score #25998

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

ansamz
Copy link
Contributor

@ansamz ansamz commented Mar 28, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

adding automatic validation to the d2_absolute_error_score metric from #24862

Any other comments?

@ansamz
Copy link
Contributor Author

ansamz commented Mar 28, 2023

@adrinjalali would appreciate your review

@glemaitre glemaitre added Validation related to input validation No Changelog Needed labels Mar 28, 2023
@glemaitre glemaitre changed the title adding validation to d2_absolute_error_score MAINT Parameter validation for d2_absolute_error_score Mar 29, 2023
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Could you add the function to the list of the functions to be tested in the file sklearn/tests/test_public_functions.py as mentioned in your other PR to validate roc_auc_score?

@@ -1499,7 +1499,17 @@ def d2_pinball_score(

return np.average(output_scores, weights=avg_weights)


@validate_params(
Copy link
Member

Choose a reason for hiding this comment

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

This should solve the issue with the linter.

Suggested change
@validate_params(
@validate_params(

Copy link
Member

Choose a reason for hiding this comment

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

This should solve the linting problem (running black would also solve this issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes absolutely! doing it right now.
I apologize for the stupid question(First time contributing), simply add it and push it on the same branch after doing the pytest?

Copy link
Member

Choose a reason for hiding this comment

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

You have to make the change on your local branch and execute pytest locally for this test_public_functions.py file. If it passes, then you can safely commit and push on your remote. This will synchronize this pull-request and our continuous integration will run your changes on all supported platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly what I did this afternoon.

@ansamz
Copy link
Contributor Author

ansamz commented Apr 3, 2023

fixed my mistakes, I hope I did it correctly this time (sorry for the mess, first time)

@glemaitre
Copy link
Member

I think that you forgot to push the changes into your remote.

fixed my mistakes, I hope I did it correctly this time (sorry for the mess, first time)

It is not a problem. Reviews and requested changes are part of the contribution experience :)

@jeremiedbb
Copy link
Member

Closing in favor of #26066

@jeremiedbb jeremiedbb closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants