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

[docs] [dask] Add return information to Dask fit() docs (fixes #4402) #4716

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

jameslamb
Copy link
Collaborator

Fixes #4402, adding Returns blocks to the documentation for fit() method on estimators in lightgbm.dask.

How I tested this

cd docs
make clean html
open _build/html/index.html

Looks like the docs are rendering correctly.

image

I also clicked through the links next to Return type and confirmed that they link back to the constructor docs for the relevant class.

@jameslamb jameslamb added the doc label Oct 26, 2021
@jameslamb jameslamb requested a review from jmoralez October 26, 2021 03:53
@jmoralez
Copy link
Collaborator

Looking at the current scikit-learn estimators (Classifier, Regressor) I see that the return type is object, however this seems better (specifying the return type as the estimator itself and linking to the constructor), should we open a new issue to change that?

Also looking at scikit-learn docs they specify that the fit method returns the fitted estimator. So maybe we could change it to something like:

    Returns
    -------
    self : lightgbm.DaskLGBMClassifier
        Fitted estimator.

WDYT @jameslamb?

@StrikerRUS
Copy link
Collaborator

@jameslamb

I also clicked through the links next to Return type and confirmed that they link back to the constructor docs for the relevant class.

Can this behavior be preserved without lightgbm. prefix in the return type?
I hope it can, given the following example

Returns
-------
booster : Booster
The trained Booster model.

https://lightgbm.readthedocs.io/en/latest/pythonapi/lightgbm.train.html#lightgbm.train
image

I think it will be good to match naming schema that is used around all other docstrings. E.g.,

Default: 'l2' for DaskLGBMRegressor, 'binary(multi)_logloss' for DaskLGBMClassifier, 'ndcg' for DaskLGBMRanker.

@StrikerRUS
Copy link
Collaborator

Ah, just looked through the codebase more carefully and it seems lightgbm.dask is already using lightgbm. prefix everywhere in docstrings for referring to its classes. Please forget my previous comment #4716 (comment) then.

@jameslamb
Copy link
Collaborator Author

Thanks for the reviews! I just updated this to latest master. If it passes, I'll merge this.

@jmoralez I agree with your suggestion to update the same information from the sklearn docstrings. I'll propose those changes in a separate PR.

@StrikerRUS
Copy link
Collaborator

What do you think about locating Returns section before a note about custom function, like it's done in sklearn?

sklearn

image

dask

image

@jameslamb
Copy link
Collaborator Author

What do you think about locating Returns section before a note about custom function, like it's done in sklearn?

Sure, works for me!

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thank you!

@jameslamb jameslamb merged commit 858cc0a into master Nov 8, 2021
@jameslamb jameslamb deleted the docs/dask-return branch November 8, 2021 01:58
@StrikerRUS StrikerRUS mentioned this pull request Jan 6, 2022
13 tasks
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python] Add Returns section to docstrings of fit() methods of Dask estimators
3 participants