Some tests (eight) fail in Microsoft.ML.Predictor.Tests

[veikkoeeva]

System information

.NET Command Line Tools (2.1.200)

Product Information:
Version: 2.1.200
Commit SHA-1 hash: 2edba8d7f1

Runtime Environment:
OS Name: Windows
OS Version: 10.0.17134
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\2.1.200\

Microsoft .NET Core Shared Framework Host

Version : 2.0.7
Build : 2d61d0b043915bc948ebf98836fefe9ba942be11

Issue

1. I checkout the project,
2. Ran .\build.cmd on PowerShell prompt
3. Opened the project in VS 2017 15.7
4. Ran all tests, some of which failed (see attached image).

I did expect all tests to pass.

The assert here fails. I didn't go further to check if the cause is a programming error or something else as this was more of curiosity, but noted here. :)

These are all of the eight tests that fail due to the assert in the previous image:

[Anipik]

@veikkoeeva can you please paste the error output ?

[veikkoeeva]

@Anipik Hmm, indeed... It failed on the assert, but there might be something written to stdout before that (a refactoring idea, might be good to assert the terms directly). I'm not on that code currently, but I'll check in about 14 hours.

[Anipik]

@veikkoeeva generally it mentions the line and the file name where the matching of the outputs failed.

[veikkoeeva]

@Anipik I took a look and tried to fix a bit, but I could lend a hand a bit. Do you happen to know where the results to LogisticRegression-bin-norm-CV-breast-cancer-rp.txt are written? This eludes me somehow. The result file looks like this

LogisticRegression
AUC	Accuracy	Positive precision	Positive recall	Negative precision	Negative recall	Log-loss	Log-loss reduction	F1 Score	AUPRC	/l2	/ot	/nt	Learner Name	Train Dataset	Test Dataset	Results File	Run Time	Physical Memory	Virtual Memory	Command Line	Settings	
0,9945	0,969373	0,959559	0,952772	0,975316	0,977394	0,134393	85,57494	0,956039	0,988987	0,1	0,001	1	LogisticRegression	%Data%		%Output%	99	0	0	maml.exe CV tr=LogisticRegression{l1=1.0 l2=0.1 ot=1e-3 nt=1} threads=- dout=%Output% data=%Data% seed=1 xf=BinNormalizer{col=Features numBins=5}	/l2:0,1;/ot:0,001;/nt:1	

and I suppose the problem is visible. In the PR I have gone through the places the code uses one-by-one and the results in some other files, such as in LogisticRegression-bin-norm-CV-breast-cancer-out.txt look consistent with the assertion data currently (though the runner still reports it as a failure, I'll check that later).

[TomFinley]

Ah that's interesting @veikkoeeva , thanks for bringing this up.

The rp files are written by the so-called ResultProcessor, the code for which lies in the src/Microsoft.ML.ResultProcessor project... so this LogisticRegression-bin-norm-CV-breast-cancer-rp.txt file, I would expect is the result of running ResultProcessor on top of the LogisticRegression-bin-norm-CV-breast-cancer-out.txt I'd expect to see alongside it.

[Anipik]

Yeah the problem is that the decimal separator in your language pack is comma instead of decimal.
and we just match the rp files using string matching

The matching is being done here https://github.com/Anipik/machinelearning/blob/master/test/Microsoft.ML.TestFramework/BaseTestPredictorsMaml.cs#L211

The fix could be https://github.com/Anipik/machinelearning/blob/master/test/Microsoft.ML.TestFramework/BaseTestBaseline.cs#L618
instead of directly using . as a data separator you can dynamically obtain the decimal separator

[Anipik]

@danmosemsft can you add a non-english queue for this repo too ?

[veikkoeeva]

@TomFinley Thanks, I'll see if get the rest fixed today (it's 19:00 here).

@Anipik Hmm, good to know. As you can see, I've tried to fix all instances that print numbers in non-invariant way. It occurred I could fix the comparison, but then one would have files that aren't easy to diff, say, when asking help here or comparing oneself. If this approach is OK, I think I should add a note to the commit about this.

My locale is fi-FI, by the way.

[danmoseley]

@danmosemsft can you add a non-english queue for this repo too ?

@Anipik feel free to open an issue, and make the addition, if it's analogous to corefx's.

[Anipik]

@veikkoeeva is this issue resolved ?

[veikkoeeva]

@Anipik Ah, sorry, I got delayed. I'm currently in www.arctic15.com and rather tied. See the linked PR for further discussion and the last comment. I should be able to get onto it the next weekend, knock on the tree. Naturally if you need to do something in the meantime, feel free to do so. :)