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

[python-package] [docs] Update key format of eval_hist in docstring example #5980

Merged
merged 5 commits into from Sep 8, 2023

Conversation

Alnusjaponica
Copy link
Contributor

Although the key format of eval_hist, which is return value of lightgbm.cv, is changed by #2089, the document is not properly updated. This PR updates docstrings of lightgbm.cv.

@jameslamb jameslamb added the doc label Jul 14, 2023
@jameslamb jameslamb changed the title Update key format of eval_hist in docstring example [python-package] [docs] Update key format of eval_hist in docstring example Jul 14, 2023
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute, and for fixing this!

While you're touching these lines, could you also replace eval_hist in this docstring with eval_results?

I think "hist" is potentially confusing in the context of LightGBM... in this docstring it's short for "history", but in other part of the project it refers to "histogram".

@Alnusjaponica
Copy link
Contributor Author

@jameslamb Thanks for the comment. I updated the name as you suggested.
I also updated the description to make it more detailed. Please let me know if it is not necessary.

@Alnusjaponica Alnusjaponica marked this pull request as ready for review July 14, 2023 23:30
@Alnusjaponica
Copy link
Contributor Author

Since eval_train_metric is not propagated to _agg_cv_result, it seems eval_results keys always have prefix "valid " or "train ". I'll confirm if it is true and update this PR.

@Alnusjaponica Alnusjaponica marked this pull request as draft July 15, 2023 00:06
@Alnusjaponica Alnusjaponica marked this pull request as ready for review July 15, 2023 00:10
@Alnusjaponica
Copy link
Contributor Author

Hi, I updated the PR but cuda 11.7.1 wheel (linux, gcc, Python 3.11) seems to have failed unexpectedly. Could you re-run the CI?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! Sorry it took so long for someone to come approve and merge it.

I double-checked today and this looks exactly correct.

import lightgbm as lgb
from sklearn.datasets import make_regression
from sklearn.model_selection import train_test_split

X, y = make_regression(n_samples=10_000)

dtrain = lgb.Dataset(X, label=y)

res = lgb.cv(
    params={
        "objective": "regression",
        "verbosity": 1
    },
    train_set=dtrain,
    num_boost_round=3,
    nfold=3,
    stratified=False,
    metrics=["rmse", "mae"],
    eval_train_metric=True
)
print(res)
{
    'train rmse-mean': [164.79008659790668, 155.4974681623088, 147.12798762863798],
    'train rmse-stdv': [0.9721940764962446, 0.8813186038273854, 0.7731215660988449],
    'train l1-mean': [131.3923423118648, 123.7241350830813, 116.79776681505548],
    'train l1-stdv': [1.1095661035927, 1.0188437396031365, 0.9319671945961616],
    'valid rmse-mean': [165.3986341202076, 156.68732293910656, 148.9184410024208], 
    'valid rmse-stdv': [2.0389944051444147, 1.968325952724342, 1.8634034991265391], 
    'valid l1-mean': [131.83160329546283, 124.72266699566386, 118.22843843154574], 
    'valid l1-stdv': [2.3549942899776246, 2.2088693238483157, 2.0893026758193805]
}

We hope you'll consider contributing more in the future 😊

@jameslamb jameslamb merged commit 6862162 into microsoft:master Sep 8, 2023
41 checks passed
@Alnusjaponica Alnusjaponica deleted the feature/update-docstring branch September 9, 2023 14:49
@neverfox
Copy link

neverfox commented Sep 14, 2023

Isn't this prefix a breaking change? I just ran into it going from 3.3.4 to 4.1.0. Where could I find that in the release notes as such?

@jameslamb
Copy link
Collaborator

@neverfox please open a new issue referencing this, and we. can discuss there. Let's not continue this discussion in the comments of an already-closed PR please.

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 Dec 20, 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.

None yet

3 participants