DOC: Fixes Docs for estimator types do not list all possible estimator types #29956
Conversation
|
Friendly ping to @Charlie-XIAO 🙂 |
There was a problem hiding this comment.
LGTM overall, thanks @DeaMariaLeon! Just a few suggestions:
There was a problem hiding this comment.
Here's the generated artifact: https://output.circle-artifacts.com/output/job/019a735d-28ba-460e-ab25-a65d7ffefbdb/artifacts/0/doc/developers/develop.html#estimator-types, LGTM. Thanks @DeaMariaLeon!
| - ``"regressor"`` for regressors | ||
| - ``"clusterer"`` for clustering methods | ||
| - ``"outlier_detector"`` for outlier detectors | ||
| - ``"DensityEstimator"`` for density estimators |
There was a problem hiding this comment.
I agree that this matches our current code base; however, I find the casing style of the last tag value inconsistent with the others. However, I don't see any easy way to fix it without breaking backward compat because it's technically part of our public API.
I guess we have to live with it for now but keep it in mind if we want to release scikit-learn 2.0 one day.
/cc @adrinjalali as developer API specialist.
There was a problem hiding this comment.
in a rework of this document, I'd like to remove all of these and only document developing estimators via inheriting from our mixins. That would remove these parts anyway.
Otherwise. also gonna work on a PR to move this from the class attribute to a tag. I think it's doable, but need to actually implement it to be sure.
There was a problem hiding this comment.
/cc @adrinjalali as developer API specialist.
I like the fancy title 😅
Reference Issues/PRs
Fixes #29900
What does this implement/fix? Explain your changes.
Any other comments?