ENH All sklearn estimators in trusted list#237
Conversation
adrinjalali
left a comment
There was a problem hiding this comment.
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. |
adrinjalali
left a comment
There was a problem hiding this comment.
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? |
adrinjalali
left a comment
There was a problem hiding this comment.
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
sklearnestimators to the trusted list ofObjectNodeand added test for it.A couple of things that I am not 100% sure about:
'sklearn.', should I add any extra ones?ObjectNodethe only place where we should add the sklearn estimators in the trusted list? What aboutTypeNode?get_tested_estimatorsin a common place (testing_utils.py) so I could reuse it. I could not use the namecommon.pybecause there is already acommon.pyinhub_utilsandpytestcomplained. Should we maybe have a common utils file for all the tests?