Skip to content

Fix the treatment of LightGbm Evaluation Metric parameters in ML.NET … #3815

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

Merged
merged 6 commits into from
Jun 12, 2019

Conversation

najeeb-kazmi
Copy link
Member

@najeeb-kazmi najeeb-kazmi commented Jun 4, 2019

…and make them conform to LightGbm

Fixes #3761 plus some related issues discovered during investigation

  1. There was a bug in LightGbm EvaluateMetricType where if a user specified EvaluateMetricType.Default, the metric would not get added to the options Dictionary, and LightGbmWrappedTraining would throw because of that.

  2. Secondly, EvaluateMetricType.Default in LightGbm is supposed to be an empty string "" and LightGbm chooses the default metric according to the problem type when this is specified. This was not present in ML.NET parameter name mappings. EvaluateMetricType.None is supposed to be "None" in LightGbm but was "" in ML.NET parameter name mappings.

  3. [Update: REVERTED] Third, in ML.NET, the default EvaluationMetric is set to EvaluateMetricType.Error for multiclass, EvaluationMetricType.LogLoss for binary, and so on. This leads to inconsistent behavior from the user's perspective: If a user specified EvaluationMetric = EvaluateMetricType.Default, the parameter passed to LightGbm would be the empty string "" but if they do not specify EvaluationMetric at all, the parameter passed to LightGbm would be Error for multiclass, LogLoss for binary, and so on.

This PR does the following:

  • Fixes the bug in (1)
  • Addresses (2) by adding all the parameters to the options dictionary with the correct values (i.e. conforming to LightGbm docs
  • [Update: REVERTED] Addresses (3) by changing the default EvaluationMetric in ML.NET to EvaluationMetricType.Default

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@ganik ganik left a comment

Choose a reason for hiding this comment

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

:shipit:

@najeeb-kazmi najeeb-kazmi merged commit 23332c1 into dotnet:master Jun 12, 2019
codemzs added a commit to codemzs/machinelearning that referenced this pull request Jul 3, 2019
… ML.NET … (dotnet#3815)"

This reverts commit 23332c1.

# Conflicts:
#	test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs
@najeeb-kazmi najeeb-kazmi deleted the 3761 branch January 30, 2020 01:22
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The given key 'metric' was not present in the dictionary
3 participants