Skip to content

[python-package] simplify eval result printing #6749

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 16, 2024
Merged

[python-package] simplify eval result printing #6749

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1db89d
[python-package] simplify eval result printing
jameslamb Dec 12, 2024
5fa72f4
Merge branch 'master' into python/format-eval-result
jameslamb Dec 12, 2024
4d446de
Update python-package/lightgbm/callback.py
jameslamb Dec 12, 2024
6b63377
better names
jameslamb Dec 14, 2024
f2012b7
Merge branch 'master' into python/format-eval-result
jameslamb Dec 14, 2024
cc9bbbd
Merge branch 'python/format-eval-result' of github.com:microsoft/Ligh…
jameslamb Dec 14, 2024
d97590d
Merge branch 'master' into python/format-eval-result
jameslamb Dec 14, 2024
132075d
Merge branch 'master' into python/format-eval-result
jameslamb Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions python-package/lightgbm/callback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,13 @@ class CallbackEnv:

def _format_eval_result(value: _EvalResultTuple, show_stdv: bool) -> str:
"""Format metric string."""
if len(value) == 4:
return f"{value[0]}'s {value[1]}: {value[2]:g}"
elif len(value) == 5:
if show_stdv:
return f"{value[0]}'s {value[1]}: {value[2]:g} + {value[4]:g}" # type: ignore[misc]
else:
return f"{value[0]}'s {value[1]}: {value[2]:g}"
else:
raise ValueError("Wrong metric value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're loosing this error, not sure if it can happen but may be worth adding a check at the start like:

if len(value) not in [4, 5]:
    raise ValueError("Wrong metric value")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, I don't think this is a very helpful error message. "Wrong metric value" doesn't really describe the problem here, and you'll end up walking the stacktrace to find this point in the code anyway.

Originally in this PR, I was thinking we could just keep this simple and let Python's "too many values to unpack" or "not enough values to unpack" tuple-unpacking errors convey that information. But thinking about it more... removing this exception means that you could now implement a custom metric function that returns tuples with more than 5 elements and LightGB would silently accept it. I think it's valuable to continue preventing that, to reserve the 6th, 7th, etc. elements for future LightGBM-internal purposes.

I'll put back an exception here using logic like you suggested... but changing the message to something a bit more helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright so I tried to add an error message here, and then write a test to check that it's raised... and I couldn't find a public code path that'd allow a tuple with too few or too many tuples to get to this point.

Here's what I tried:

import lightgbm as lgb
from sklearn.datasets import make_regression

X, y = make_regression(n_samples=10_000, n_features=3)

def constant_metric(preds, train_data):
    # returns 4 elements (should be 3)
    return ("error", 0.0, False, "too-much")

dtrain = lgb.Dataset(X, label=y).construct()
dvalid = dtrain.create_valid(X, label=y)

bst = lgb.train(
    params={
        "objective": "regression"
    },
    train_set=dtrain,
    feval=[constant_metric],
    valid_sets=[dvalid],
    valid_names=["valid0"]
)

That gets stopped earlier:

[LightGBM] [Info] Total Bins 765
[LightGBM] [Info] Number of data points in the train set: 10000, number of used features: 3
[LightGBM] [Info] Start training from score -0.471765
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jlamb/miniforge3/envs/lgb-dev/lib/python3.11/site-packages/lightgbm/engine.py", line 329, in train
    evaluation_result_list.extend(booster.eval_valid(feval))
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jlamb/miniforge3/envs/lgb-dev/lib/python3.11/site-packages/lightgbm/basic.py", line 4445, in eval_valid
    return [
           ^
  File "/Users/jlamb/miniforge3/envs/lgb-dev/lib/python3.11/site-packages/lightgbm/basic.py", line 4448, in <listcomp>
    for item in self.__inner_eval(self.name_valid_sets[i - 1], i, feval)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jlamb/miniforge3/envs/lgb-dev/lib/python3.11/site-packages/lightgbm/basic.py", line 5214, in __inner_eval
    eval_name, val, is_higher_better = feval_ret
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: too many values to unpack (expected 3)

Here:

LightGBM/python-package/lightgbm/basic.py

Lines 5209 to 5215 in b33a12e

feval_ret = eval_function(self.__inner_predict(data_idx), cur_data)
if isinstance(feval_ret, list):
for eval_name, val, is_higher_better in feval_ret:
ret.append((data_name, eval_name, val, is_higher_better))
else:
eval_name, val, is_higher_better = feval_ret
ret.append((data_name, eval_name, val, is_higher_better))

So I think that maybe this error message we're talking about was effectively unreachable.

And I don't think we should add a custom and slightly more informative error message there in Booster.__inner_eval():

  • that code will raise a ValueError if anything other than exactly a 3-item tuple (or list of such tuples) is returned... so no need to add more protection against tuples of other sizes
  • that part of the code is already kind of complicated
  • that part of the code runs on every iteration, so adding an if len() ..: raise would add a bit of extra overhead to every iteration

eval_name, metric_name, eval_result, *_ = value
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of using similar names to the ones here

LightGBM/python-package/lightgbm/basic.py

Line 5215 in 53e0ddf

ret.append((data_name, eval_name, val, is_higher_better))

I find the first one a bit confusing, since it's the name of the validation set. Maybe something like dataset_name, metric_name, metric_value, *_ = value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're totally right... I think eval_name and eval_result are confusing. I love your suggested names, agree we should standardize on those (here and in future PRs like this).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated in 6b63377

I'll do more of this renaming in future PRs.

out = f"{eval_name}'s {metric_name}: {eval_result:g}"
# tuples from cv() sometimes have a 5th item, with standard deviation of
# the evaluation metric (taken over all cross-validation folds)
if show_stdv and len(value) == 5:
out += f"+ {value[4]}"
return out


class _LogEvaluationCallback:
Expand Down
Loading