Third Party Estimator Wrapper #1103
Conversation
Adds a wrapper for estimators that implement the scikit-learn API but do not extend BaseEstimator. If the estimator is missing required properties (generally the learned attributes) then a sensible error is raised. Includes documentation about how to use non-sklearn estimators with Yellowbrick. Fixes #1098 Fixes #1099 Related to #397 Related to #1066
bbengfort
changed the title
There was a problem hiding this comment.
Looks great @bbengfort; thank you for taking this on (especially given it's been on our wishlist for >2 years now!) I've noted just a handful of minor comments inline.
One other thought — I think that this PR should close #397 as well as #1098 and #1099; if there are specific libraries that folks want to add support for, I propose opening those as separate issues (e.g. #1066), given the potential for variations in external API behaviors, which may be easier to tackle individually (that odd catboost thing you noticed, for instance).
Once we've merged this, I might do a small follow-on PR to update our FAQ page, since this would be a great new addition!
| if isinstance(model, Pipeline): | ||
| return get_model_name(model.steps[-1][-1]) | ||
| elif isinstance(model, ContribEstimator): | ||
| return model.estimator.__class__.__name__ | ||
| else: | ||
| if isinstance(model, Pipeline): | ||
| return get_model_name(model.steps[-1][-1]) | ||
| else: | ||
| return model.__class__.__name__ | ||
| return model.__class__.__name__ |
There was a problem hiding this comment.
Do we think this change needs a new test?
There was a problem hiding this comment.
Model and Pipeline are tested in tests/test_utils/test_helpers.py lines 51-86 and ContribEstimator in tests/test_contrib/test_wrapper.py line 96; so the function is completely covered - but would you prefer that all of these tests were in one place?
|
@rebeccabilbro adding this to the FAQ would be great, thanks! Closing #397 works for me- I'll add a couple of issues for libraries that I think we should cover; possibly as spike issues so that interested folks can take them on one by one. Let me know about the tests and I can merge it in; or feel free to merge when you're ready! |
|
@rebeccabilbro Created #1107 #1106 and #1105 in response to your suggestion about closing #397 |
Adds a wrapper for estimators that implement the scikit-learn API but do not extend BaseEstimator. If the estimator is missing required properties (generally the learned attributes) then a sensible error is raised. Includes documentation about how to use non-sklearn estimators with Yellowbrick.
Fixes #1098
Fixes #1099
Fixes #397
Related to #1066
I have made the following changes:
ContribEstimatorwrapper and helper functions to wrap cuML, catboost, neuraxle etc estimatorsis_classifieris_estimatorandget_model_namehelpers to use theContribEstimatoryellowbrick/contrib/__init__.pyStill to do:
CHECKLIST
Included a sample plot to visually illustrate your changes?Have you updated the baseline images if necessary?pytest?make html?