Fixes data invariant format problems

[veikkoeeva]

The tests do not pass on machines that have different formatting than English language. The error happens since the results are written in different than expected format.

Fixes #74

[TomFinley]

Thanks for writing code to fix the tests @veikkoeeva !

A more philosophical question, one I don't know the answer to, maybe you or other reviewers could chime in:

For things like output format data it's important that we write that in the invariant culture (since it would be weird if someone wrote data from this library, then someone else was unable to read it, merely because they were in a different region). For this reason, I'd say (just to pick an example), the changes to Ensemble.cs and RegressionTree.cs are unambiguously good changes, since the primary purpose of that code is to write out a format intended to be interpreted by some other program.

I start to become slightly more uncomfortable, with the logs (again, just to pick one single example, the changes in EvaluatorUtils.cs). The logs are meant to be read by a human and is not intended to be any sort of interchange format (code like ResultProcessor notwithstanding, which exists primary for the sake of the tests). If someone's culture declares the decimal separator to be , rather than the en-US-style ., then my first reaction would be to just go with what I must consider the user's stated preference. That is, I'd consider this to be a fault in the tests, rather than a fault in the software being tested.

I wonder if, while keeping your changes, it is possible to "solve" this problem, in the baseline tests at least, setting the culture explicitly to en-US or the invariant "" in the test initializer (as [described here] (https://stackoverflow.com/questions/5001225/c-sharp-how-can-i-force-localization-culture-to-en-us-for-tests-project) for en-US). Then real usage of the tool would continue to output logs in the user's culture, but at least the tests would run uniformly.

[veikkoeeva]

@TomFinley Good points. There are other problems too, I put https://github.com/dotnet/machinelearning/pull/109/files#diff-8be2040d2cb5bab8e8be76992fc763c9R179 since otherwise it would take the results as string, parse it to a float and have a completely different result when again printed out as string (if I recall correctly). I'm not sure if this would handle such a situation, but let's pause and think.

Should we fix this now in some particular fashion and open a few more tickets for further refactoring? See previous point about Optimizer -- and your point may work. I'll check and see.

This is how I would like to change the system:

Have clearly separated data structures for computation and results, they're likely mostly there. Then have some sort of a separate formatter to the results to desired format. This might require something that traverses the data structure and then format individual entries while traversing, ideally writing to the sink as it goes so that if the sink doesn't accumulate results, they get written out immediately.

Then the tests would use exactly the same facilities as any other code. As for example classes that might be useful to frame discussion, perhaps there's room to introduce IFormatProvider (one another exampe here).

To do the aforementioned, one would need to separate code that logs/traces for "technical purpose" and then to write a result. I.e. write result clearly in the end as in choosing a suitable Func that handles the dataset that has the resultset. This writer would take a formatter and in that case when one checks test, the data structures can be compared in-memory for correctness and then it's a separate test to compare writing the results is according to some other criteria. This would help a lot tool writers too, I think.

I think the data structures are like this gist: https://gist.github.com/veikkoeeva/50c8f38ec46b0a3ce70467d16af00dc1. If the columns there would be heterogenous (with type information), it could be processed with other standard facilities. Then one could write something like at https://stackoverflow.com/questions/2758850/make-csv-from-list-of-string-in-linq.

The idea is to make plain data structures and separate printing and other handling logic off of them.

I apologize also as I likely come across wrongly here. I'm unfamiliar with the code, but to throw around some ideas. I'm unfortunately somewhat busy for the next few weeks, but I think this would be nice discussion to have together with logging/tracing.

[veikkoeeva]

A quick note, latest changes to ResultProcessor brought this close:

[veikkoeeva]

@TomFinley Excellent suggestion. The tests are green with all the changes you see now and in addition to

I'll remove changes and see how well this change holds, taking a note of Ensemble and others. While I think the real fix is refactoring the code so one can write out the result set with a chosen culture and otherwise taking into consideration the larger logging issues (see at #51 (comment)), the less invasive the change is the less invasive it is -- and in this case perhaps even more robust for this purpose. :)

<edit: Changed locale switch in tests to the base class instead of the inherited one.

[TomFinley]

Nit: Sort order?

[TomFinley]

Sort order?

[TomFinley]

Sort order?

[TomFinley]

Cool, sounds great -- yes makes sense, thank you!

---

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

[TomFinley]

Thanks again @veikkoeeva , is your note about thinking it's inappropriate to merge still current?

[veikkoeeva]

@TomFinley Don't merge yet. :) I had to sleep and need to take care day job in between, but I should get the test today (likely you should have day there at that day).

[eerhardt]

The logs are meant to be read by a human and is not intended to be any sort of interchange format (code like ResultProcessor notwithstanding, which exists primary for the sake of the tests). If someone's culture declares the decimal separator to be , rather than the en-US-style ., then my first reaction would be to just go with what I must consider the user's stated preference.

Should we explicitly be passing the "CurrentCulture" to those places? That way it is marked as "I thought about the usage of this formatting, and it is intended for human consumption."
Reading the current code, we only go through and mark certain formattings as "InvariantCulture" in this change.

[veikkoeeva]

@eerhardt The problem was/is the results are printed out to a file in different formating as the ones that are used in the control set for comparison. Imbuing en-US when starting the tests makes the system to write out the result file apparently in the same culture as the comparison file. Other way of seeing this is that that the control files are written in invariant culture, which implies the results should be written also using invariant culture.

I don't think it's a correct idea to think these files should print in the locale of the user. Firstly, the text is in English either way so having numbers, dates and so on in different local would print them out in that way whereas currently there are these %things%. Secondly, in many places the user may want to explicitly choose localization when needed and it may not be the one the threas has (e.g. international companies may mandate English but some other language would be preferred). Thirdly, if there's a problem, I'd be diffing to find the differences and this would be more difficult this way. Fourthly, it seems a bit laborius to apply fixes in "just the places that make the tests pass" (which, though, more or less was what I did) while the en-US imbued culture could mask problems if that and also CurrentCulture would be used and it looks like changes to code, new tests, refactoring and so on, could introduce again similar problems.

It looks like partially the problem here is also a separation of what is tracing and logging and what should be writing results. This is something I may get wrong, but writing to output "here and there" and type transformations are a bit of a problem. Hence the currently in the commit message it reads

Fixes data invariant format problems

The tests do not pass on machines that have different formatting than
English language. The error happens since the results are written in different
than expected format.

1. The main fix is to imbue en-US culture to the test thread so that results
will be output in format that is comparable with the test format.

2. A secondary fix is to make comparisons between culture sensitive data type
representations invariant when they do not have human readable dimensions. In
OptimizationMonitor.cs case the cast between culture sensitive floating point
and string will cause orders of magnitudes of error in output results.

The intention of this path is not to offer a robust solution and remove future
issues. There is room for refactoring where, for instance, locale information
would be applied to input and output and logging/tracing would be clearly
separated from another kind of locale sensitive handling. This way culture
sensitive parts would be separated and particular output formats could be
tested as separate cases if so desired.

[veikkoeeva]

@eerhardt Structured logging would nice, by the way. :) And all those places that use StringBuilder to mash up texts could perhaps be dropped into logger given via dependency injection. I'll continue here a bit, when I applied the fixes in one place, I noticed in the output file some othe places had it still printed using fi-FI locale, so I searched for the next place to check. Especially the IProgress based system was easy to miss since it wasn't easy to see from the output nor in the debugger why the output was like it was.

Hence the comment it'd be easier to debug if one had the result as a set and then just compared it in the end and if needed, output using given locale. I would expect this is something the users of the libraries want to do, though. If there's something like .ToCsv(this DataStructure, IEnumerable<string> columnHeaders, IFormat..., then having that take locale or some other IFormatProvider could be nice and maybe worth testing

[veikkoeeva]

@TomFinley I'm off to sleep now. I'll be around tomorrow (and the weekend) if this warrants more discussion or other changes (I might have missed some feedback too). It's OK to me to step back and consider the cases as much as needed. :)

[danmoseley]

@eerhardt are more changes requested here? I'm unclear.

@veikkoeeva looks like you will have to rebase/merge master

[veikkoeeva]

@danmosemsft A good question. It could be just imbuing the locale will unblock the tests, but I'm not sure if it'd leave runtime problems (as I had to change the Optimizer too). Without imbuing the locale almost all the test passes just by changing the output to invariant culture, but a few issues remained. So the options could be:

1. Push this in regardless.
2. I try tomorrow to just imbue with the locale and see if the tests are green and a separae issue is logged for the invariant cases in genera.
3. I fix the rest of logging without imbuing locale.
4. I fix the rest of logging and also imbue locale as an added robustness measure (which could also mask problems in non-test related code).

I can rebase too tomorrow. It's almost midnight here, hence pushing for tomorrow. :)

[eerhardt]

I'm not sure I'm convinced that all the places that now pass in InvariantCulture are right. For example:

https://github.com/dotnet/machinelearning/pull/109/files#diff-9601d51e7060222818c0aab206368121R50

 _ch.Info("{0}\t Time elapsed(s): {1}\n\n", DateTime.Now.ToString(CultureInfo.InvariantCulture), elapsedSeconds.ToString(CultureInfo.InvariantCulture));

This really looks like something that a user would be reading, and not another program. Thus, it shouldn't be using the InvariantCulture. (Although, obviously we are only using English words here, so localizing these messages would also be necessary if we wanted to be correct for any user.)

[veikkoeeva]

@eerhardt

This really looks like something that a user would be reading, and not another program.

Might be more correct to use, say, en-US there in that case. This change was with the assumption the result files used to diff are in "invariant culture" and without imbuing the locale. I wrote about testing a bit differently earlier in the the thread, so I refer to it (for the casual reader who mightn't noticed).

[eerhardt]

Might be more correct to use, say, en-US there in that case.

I disagree. In the cases where we are writing to a log file that a human will read, it is better to write using the current culture, that way the format of the date is written in the format the current user is used to. For example, in the US it is common to have MM/dd/YYYY formats, but most countries don't use this format. This code shouldn't be hard-coding a culture at all (Invariant or otherwise). It should be using the current culture.

For testing - I agree we can fix our tests by explicitly using a hard-coded culture (which you've done). But this change should only happen in the tests. All the changes in the product code, should be doing what is best for the user and the product.

Can you revert the changes you made to the product code and just leave the test changes? Does that fix the issue?

Also, there are now conflicts that need to be resolved. Can you take care of that?

[veikkoeeva]

@eerhardt I'll revert changes to non-test code. Most likely I'll just try imbuing the locale in tests and see if it fixes the problems of tests not passing. If not, have to see why not.

[veikkoeeva]

@eerhardt I updated to imbue locale only. It seems to fix the tests, but see the notes I don't think this guarantees runtime is OK under different locales. Might be, but it's not sure and the fix kind of masks the problem if there are. If so, maybe that's only temporary.

I disagree. In the cases where we are writing to a log file that a human will read, it is better to write using the current culture, that way the format of the date is written in the format the current user is used to.

I disagree with that. I like to read consistently in one language so I don't have the mental load of multiple ones in the same text. Maybe also, if human readable reporting is the thing here, produce JSON and have a parser and another tool for viewing that output.

But it looks to me the discussion should be about comparing in-memory data structures and then optionally test files are written according to some locale. Maybe there should be tests reading data in user specified locale and printing in user specified locale and then a comparison test to see if the results were valid.

[eerhardt]

Thanks for taking care of this, @veikkoeeva. This looks good to me.

[eerhardt]

I like to read consistently in one language so I don't have the mental load of multiple ones in the same text

My comment wasn't referring to language, but it was referring to the format of the dates. Should the month go first? Or should the day go first? There are plenty of countries that use English as the language, but don't use the US date format.

[shauheen]

Thanks @veikkoeeva , @eerhardt and @TomFinley. I will merge this tomorrow.

[asclearuc]

Looks like current solution doesn't work always.
At least all 2 computers I work on - it doesn't help.
To solve it, I had to indicate date format explicitly:

diff --git a/test/Microsoft.ML.TestFramework/BaseTestClass.cs b/test/Microsoft.ML.TestFramework/BaseTestClass.cs
index d4809721..66ef2cf5 100644
--- a/test/Microsoft.ML.TestFramework/BaseTestClass.cs
+++ b/test/Microsoft.ML.TestFramework/BaseTestClass.cs
@@ -50,6 +50,7 @@ namespace Microsoft.ML.TestFramework
//files can be compared on systems with other locales to give set of known
//correct results that are on en-US locale.
Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US");
+ Thread.CurrentThread.CurrentCulture.DateTimeFormat.ShortDatePattern = "dd-MM-yyyy";

#if NETFRAMEWORK
string codeBaseUri = typeof(BaseTestClass).Assembly.CodeBase;