[python-package] stop relying on string concatenation / splitting for cv() eval results#6761
Merged
[python-package] stop relying on string concatenation / splitting for cv() eval results#6761
Conversation
jameslamb
commented
Dec 17, 2024
| metric_type[key] = one_line[3] | ||
| cvmap.setdefault(key, []) | ||
| cvmap[key].append(one_line[2]) | ||
| return [("cv_agg", k, float(np.mean(v)), metric_type[k], float(np.std(v))) for k, v in cvmap.items()] |
Member
Author
There was a problem hiding this comment.
This, removing this "cv_agg" string literal, is the key change... everything else flows from that.
StrikerRUS
requested changes
Dec 17, 2024
Collaborator
StrikerRUS
left a comment
There was a problem hiding this comment.
LGTM! Thanks a lot for clear refactoring!
Just some minor suggestions.
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
…into python/remove-cv-agg
StrikerRUS
approved these changes
Dec 19, 2024
Collaborator
StrikerRUS
left a comment
There was a problem hiding this comment.
LGTM! Thank you for working on this and for taking my comments into account!
Contributor
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contributes to #6748
There are a few activities where
lightgbm(the Python package) needs to inspect the output of one or more evaluation metrics on one or more datasets.For example:
For
train()and other APIs that end up using it, it tracks those using a list of tuples like this (pseudocode):cv()does something similar. However, its "metric value" is actually a mean of such values taken over all cross-validation folds. Because multiple values are being aggregated, it appends a 5th item with the standard deviation.Some code in
callbacks.pyneeds to know, given a list of such tuples, whether they were produced by cross-validation or regulartrain().To facilitate that while still somewhat preserving the schema for the tuples, the
cv()code:"cv_agg"to the beginning of the tupleSo e.g.
("valid1", "auc", ...)becomes("cv_agg", "valid1 auc", ...). That happens here:https://github.com/microsoft/LightGBM/blob/480600b3afaf2a0a6f32cf417edf9567f625b2c3/python-package/lightgbm/engine.py#L580-L592
Every place dealing with such tuples then needs to deal with that, including splitting and re-combining that second element. Like this:
https://github.com/microsoft/LightGBM/blob/480600b3afaf2a0a6f32cf417edf9567f625b2c3/python-package/lightgbm/callback.py#L416-L418
This proposes changes to remove that, so that the
cv()andtrain()tuples follow a similar schema and all the complexity of splitting and re-combining names can be removed.It also standardizes on the names from #6749 (comment)
Notes for Reviewers
This change should be completely backwards-compatible, including with user-provided custom metric function. The code paths here are well-covered by tests (as I found out from many failed tests while developing this 😅 ).