-
Notifications
You must be signed in to change notification settings - Fork 54
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
ENH All sklearn estimators in trusted list #237
ENH All sklearn estimators in trusted list #237
Conversation
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.
Thanks for the PR @EdAbati , this looks great.
@adrinjalali You are most qualified to review this, would you take it?
I don't think we need a "common" EDIT: Wrote this before seeing Adrin's reply. |
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.
It's definitely worth having a way to get other types which are not trusted by this PR, but I don't think we should have that list in the codebase.
Hi @adrinjalali , I tweaked the unittest it looks nicer now, thanks :) Regarding instead:
do you have anything in mind? |
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.
This LGTM. Thanks @EdAbati . Could you please also add an entry in our changelog?
Thank you @adrinjalali for the help! :) |
Fixes #223
Hi all, I have added all
sklearn
estimators to the trusted list ofObjectNode
and added test for it.A couple of things that I am not 100% sure about:
'sklearn.'
, should I add any extra ones?ObjectNode
the only place where we should add the sklearn estimators in the trusted list? What aboutTypeNode
?get_tested_estimators
in a common place (testing_utils.py
) so I could reuse it. I could not use the namecommon.py
because there is already acommon.py
inhub_utils
andpytest
complained. Should we maybe have a common utils file for all the tests?