Set culture to culture invariant in LightGBM #454

codemzs · 2018-06-29T20:30:16Z

fixes #440

Ivanidzo4ka · 2018-06-29T20:37:29Z

src/Microsoft.ML.LightGBM/LightGbmTrainerBase.cs


        protected LightGbmTrainerBase(IHostEnvironment env, LightGbmArguments args, PredictionKind predictionKind, string name)
        {
+            Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US");


Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US"); [](start = 12, length = 63)

This is nice, and solves all the current problems, but are you sure this is a separate thread we spawn and not are user thread?
I can image users surprise if they code just stop working because somewhere in separate library you setup current thread culture to en-us :) #Resolved

This is current thread at the start of the argument parsing on line 71.

In reply to: 199274365 [](ancestors = 199274365)

Reactivating. Just to be clear since it's not obvious, changing a user's culture is not an appropriate solution.

In reply to: 199275306 [](ancestors = 199275306,199274365)

ok, let me give you example:
TrainAndPredictIrisModelWithStringLabelTest

Thread.CurrentThread.CurrentCulture = new System.Globalization.CultureInfo("fr-FR");
double v = 1.5f;
string t = v.ToString();

put this in begining of test

replace learner with LightGBM and check what you get same 1.5 as parse result

v = double.Parse(t);
pipeline.Add(new LightGbmClassifier());

run training:
PredictionModel<IrisDataWithStringLabel, IrisPrediction> model = pipeline.Train<IrisDataWithStringLabel, IrisPrediction>();
v = double.Parse(t);

check value in v

In reply to: 199275306 [](ancestors = 199275306,199274365)

proper solution is
a) change all places where we work with Option argument from ToString(CultureInfo("en-US")) preferably move culture to const variable
b) report this issue to LightGBM folks: https://github.com/Microsoft/LightGBM/blob/ba3e1ff24fd43ed33d7703881d8efad83a5ed452/include/LightGBM/utils/common.h#L174 people who do they own parsing mechanism, have special place in hell.
c) put comments which describe what this is temporary solution and ToString can be reverted to normal state as soon as LightGBM folks fix their parser.

It's a few lines of manual work, but it's a right way.

In reply to: 199277210 [](ancestors = 199277210,199275306,199274365)

microsoft/LightGBM#1481
here is issue to track

In reply to: 199279475 [](ancestors = 199279475,199277210,199275306,199274365)

Thanks for opening the issue on LightGBM repo.

In reply to: 199284806 [](ancestors = 199284806,199279475,199277210,199275306,199274365)

Ivanidzo4ka · 2018-06-29T20:38:47Z

src/Microsoft.ML.LightGBM/LightGbmTrainerBase.cs

 using Microsoft.ML.Runtime.Internal.Utilities;
 using Microsoft.ML.Runtime.Training;
 using Microsoft.ML.Runtime.FastTree.Internal;
+using System.Globalization;


order #Resolved

TomFinley

Hi @codemzs thanks for looking at this. Solve this another way please -- changing a user's culture is not something we should do in code called by the user...

Ivanidzo4ka · 2018-06-29T21:48:37Z

src/Microsoft.ML.LightGBM/LightGbmTrainerBase.cs

-using System.Threading.Tasks;
 using System.Collections.Generic;
+using System.Globalization;
+using System.Linq;


do you really need linq? #Resolved

yeah there are references to it in this file. Last()

In reply to: 199288120 [](ancestors = 199288120)

Reactivating. This is actually pretty bad. We ought to change that code to get the last index's value. I know it's not your fault, but, let's not let bad code stand, if we know where it is.

In reply to: 199289028 [](ancestors = 199289028,199288120)

Ivanidzo4ka · 2018-06-29T22:00:38Z

src/Microsoft.ML.LightGBM/LightGbmRegressionTrainer.cs

        protected override void CheckAndUpdateParametersBeforeTraining(IChannel ch, RoleMappedData data, float[] labels, int[] groups)
        {
-            Options["objective"] = "regression";
+            Options["objective"] = "regression".ToString(CultureInfo.InvariantCulture);


"regression".ToString(CultureInfo.InvariantCulture); [](start = 35, length = 52)

I have so many questions... #Resolved

99 questions but ... aint one?

In reply to: 199290138 [](ancestors = 199290138)

TomFinley · 2018-06-29T23:15:52Z

src/Microsoft.ML.LightGBM/LightGbmArguments.cs

-            res[GetArgName(nameof(MaxCatThreshold))] = MaxCatThreshold.ToString();
-            res[GetArgName(nameof(CatSmooth))] = CatSmooth.ToString();
-            res[GetArgName(nameof(CatL2))] = CatL2.ToString();
+            res["label_gain"] = CustomGains.ToString(CultureInfo.InvariantCulture);


CustomGains.ToString [](start = 32, length = 20)

Anything that is a string, there is of course no need to call this. Hopefully it is obvious from intellisense and whatnot what those things are. #Resolved

TomFinley · 2018-06-29T23:18:01Z

src/Microsoft.ML.LightGBM/LightGbmMulticlassTrainer.cs

        protected override void GetDefaultParameters(IChannel ch, int numRow, bool hasCategorical, int totalCats, bool hiddenMsg=false)
        {
            base.GetDefaultParameters(ch, numRow, hasCategorical, totalCats, true);
            int numLeaves = int.Parse(Options["num_leaves"]);


int.Parse(Options["num_leaves"]); [](start = 28, length = 33)

If you're doing it one way, you must also do it the other way. #Resolved

TomFinley · 2018-06-29T23:26:51Z

There's a fairly obvious way this can be improved. Options is a map of string to string. Because of that, you have to format the message every time a parameter is set. If instead it was changed to a map from string to object, you could have had the conversion to string be in exactly one place, rather than literally every single place you are assigning a value to this. Then whatever is responsible for actually passing the strings to LightGBM should be responsible for doing string.Format(CultureInfo.InvariantCulture, "{0}", foo) for each value... that is, make it part of a loop, rather than doing it every single time. #Resolved

codemzs · 2018-07-02T18:32:29Z

Thanks, Tom! This is a great feedback and indeed results in a more maintainable code.

In reply to: 401497686 [](ancestors = 401497686)

Ivanidzo4ka · 2018-07-02T19:41:54Z

src/Microsoft.ML.Data/Transforms/ConcatTransform.cs


        public sealed class Arguments : TransformInputBase
        {
-            public Arguments()


public Arguments() [](start = 12, length = 18)

I think something terribly wrong happen with your merge #Resolved

Yes, I am aware and fixing it via interactive rebase.

In reply to: 199600627 [](ancestors = 199600627)

Ivanidzo4ka · 2018-07-02T19:55:50Z

src/Microsoft.ML.LightGBM/LightGbmMulticlassTrainer.cs

        {
            base.GetDefaultParameters(ch, numRow, hasCategorical, totalCats, true);
-            int numLeaves = int.Parse(Options["num_leaves"]);
+            int numLeaves = int.Parse(string.Format(CultureInfo.InvariantCulture, "{0}", Options["num_leaves"]));


int.Parse(string.Format(CultureInfo.InvariantCulture, "{0}", Options["num_leaves"])); [](start = 28, length = 85)

why can't you just call int.Parse(Options["num_leaves"],CultureInfo.InvariantCulture); ? #Resolved

That won't work. You will need to convert object to string first and even then what advantage does this give over what I have now? I'm also concerned that we will convert to string without invariant and then try to parse with invariant flag.

In reply to: 199603671 [](ancestors = 199603671)

It would work if your Options["num_leaves"] was Invarianted string. But in your case Option["num_leaves"] contains already int value which you hide behind object. So all you need to is just a cast.
int numLeaves = (int)Options["num_leaves"];

In reply to: 199605092 [](ancestors = 199605092,199603671)

That will work but here I don't want to make an assumption that num_leaves is int, what if tomorrow someone inserts num_leaves as string or some other type? I'd rather have a generic way to translate to invariant string and then parse. What do you think?

In reply to: 199607828 [](ancestors = 199607828,199605092,199603671)

I have a proposal which will benefit all of us and remove any concerns regarding culture. Don't do parsing at all. Just set numLeaves as out parameter in the function and set it properly in base call.

In reply to: 199611982 [](ancestors = 199611982,199607828,199605092,199603671)

In this case casting to int is fine. I spoke with Tom and he mentioned we can make this hard assumptions if the datastructure/code is internal.

In reply to: 199630347 [](ancestors = 199630347,199611982,199607828,199605092,199603671)

TomFinley · 2018-07-02T20:35:32Z

src/Microsoft.ML.LightGBM/WrappedLightGbmTraining.cs

-            if (earlyStoppingRound > 0 && (parameters["metric"] == "auc"
-                || parameters["metric"] == "ndcg"
-                || parameters["metric"] == "map"))
+            if (earlyStoppingRound > 0 && (parameters["metric"].ToString() == "auc"


parameters["metric"].ToString() [](start = 43, length = 31)

What are these originally?

If they're strings, just use a direct cast. That communicates that you expect them to actually be strings. ToString() suggests there's some conversion going on. #Resolved

Here it makes to cast to string because a string is represented as a string unlike int, double, etc that can be represented in their original type or as strings.

In reply to: 199613266 [](ancestors = 199613266)

TomFinley · 2018-07-02T20:48:32Z

src/Microsoft.ML.LightGBM/LightGbmTrainerBase.cs

        private static List<string> ConstructCategoricalFeatureMetaData(int[] categoricalFeatures, int rawNumCol, ref CategoricalMetaData catMetaData)
        {
            List<int> catBoundaries = GetCategoricalBoundires(categoricalFeatures, rawNumCol);
-            catMetaData.NumCol = catBoundaries.Count() - 1;


Count() [](start = 47, length = 7)

Ah very good. Yes that is terrible. There really should be a warning or something on Count()... :P #Resolved

TomFinley

zeahmed · 2018-07-02T21:27:45Z

src/Microsoft.ML.LightGBM/WrappedLightGbmTraining.cs

-            if (earlyStoppingRound > 0 && (parameters["metric"] == "auc"
-                || parameters["metric"] == "ndcg"
-                || parameters["metric"] == "map"))
+            if (earlyStoppingRound > 0 && ((string)parameters["metric"] == "auc"


Same thing is being casted three times. it would be nice to do something like this

var metric = (string)parameters["metric"] if (earlyStoppingRound > 0 && (metric == "auc" || metric == "ndcg" || metric == "map")) ``` #Resolved

zeahmed

Looks Good! I have left a small suggestion.

* Make options culture agnostic.

codemzs assigned Ivanidzo4ka and TomFinley and unassigned Ivanidzo4ka and TomFinley Jun 29, 2018

codemzs requested review from Ivanidzo4ka and TomFinley June 29, 2018 20:30

Ivanidzo4ka reviewed Jun 29, 2018

View reviewed changes

TomFinley suggested changes Jun 29, 2018

View reviewed changes

Ivanidzo4ka reviewed Jun 29, 2018

View reviewed changes

TomFinley reviewed Jun 29, 2018

View reviewed changes

codemzs changed the title ~~Set culture to en-us in LightGBM~~ Set culture to culture invariant in LightGBM Jul 2, 2018

Ivanidzo4ka reviewed Jul 2, 2018

View reviewed changes

Make options culture agnostic.

c92009f

codemzs force-pushed the lightgbm branch from 71eeb44 to c92009f Compare July 2, 2018 20:24

TomFinley reviewed Jul 2, 2018

View reviewed changes

PR feedback.

099a0af

TomFinley reviewed Jul 2, 2018

View reviewed changes

TomFinley approved these changes Jul 2, 2018

View reviewed changes

codemzs requested a review from zeahmed July 2, 2018 20:56

zeahmed reviewed Jul 2, 2018

View reviewed changes

zeahmed approved these changes Jul 2, 2018

View reviewed changes

PR feedback.

17ec59c

codemzs merged commit 7c536ae into dotnet:master Jul 2, 2018

codemzs deleted the lightgbm branch July 2, 2018 22:16

eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018

Set culture to culture invariant in LightGBM (dotnet#454)

4328c3f

* Make options culture agnostic.

ghost locked as resolved and limited conversation to collaborators Mar 30, 2022

Set culture to culture invariant in LightGBM #454

Set culture to culture invariant in LightGBM #454

Uh oh!

Conversation

codemzs commented Jun 29, 2018

Uh oh!

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jun 29, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codemzs commented Jul 2, 2018

Uh oh!

Ivanidzo4ka Jul 2, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 2, 2018 • edited by codemzs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Jun 29, 2018 •

edited

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited

Loading

Ivanidzo4ka Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

TomFinley Jun 29, 2018 •

edited by codemzs

Loading

TomFinley commented Jun 29, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Jul 2, 2018 •

edited by codemzs

Loading

TomFinley Jul 2, 2018 •

edited by codemzs

Loading

TomFinley Jul 2, 2018 •

edited by codemzs

Loading

zeahmed Jul 2, 2018 •

edited by codemzs

Loading