Skip to content
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

DOC revamp how to develop sklearn estimator page #30253

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

adrinjalali
Copy link
Member

The way people should develop estimators has changed quite a lot in the past years, and we also have developed our developer API quite a bit in recent years.

This PR is an effort to make the guide on how to develop an estimator more up to date with what we expect users to do these days.

Copy link

github-actions bot commented Nov 8, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 9f2e735. Link to the linter CI: here

@adrinjalali adrinjalali marked this pull request as ready for review November 10, 2024 16:31
@adrinjalali
Copy link
Member Author

Marked this as ready for review @glemaitre @adam2392 . I think these are the crucial areas which we better get in for the release since things have changed, and I'd be working on improving these / the rest of the document, and adding proper examples on how to write estimators in separate PRs.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

This is already an improvement as is, so +1 from me.

very few places, only in some meta-estimators, where the sub-estimator(s) argument is
a required argument.

The arguments should all correspond to hyperparameters describing the model or the
Copy link
Member

Choose a reason for hiding this comment

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

Not all arguments are hyparmeters or part of the optimization problem. For example CountVectorizer.input is not in a search space for tuning a model.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was text from before, but I agree, changed now.

doc/developers/develop.rst Show resolved Hide resolved
@adrinjalali adrinjalali merged commit 7387c26 into scikit-learn:main Nov 11, 2024
30 checks passed
@adrinjalali adrinjalali deleted the doc/develop branch November 11, 2024 08:57
jeremiedbb pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants