Enabling FFM tests #1206

sfilipi · 2018-10-09T22:17:31Z

Resolves part of #404

wschin · 2018-10-09T22:43:46Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

+            // see https://github.com/dotnet/machinelearning/issues/404
+            // in Linux, the clang sqrt() results vary highly from the ones in mac and Windows. 
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
+                RunAllTests(binaryPredictors, binaryClassificationDatasets, digitsOfPrecision:4);


Does it mean that we only check "9487" in "9487.05"? The issue you opened said that the difference happens at the 17th decimal, so probably we can increate it to 7? #Pending

In the small subset that I explored, there were differences starting on the 4th decimal digit, for Linux. Should update the issue.

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

giving 5 another try after updating the comparison code.

In reply to: 224267254 [](ancestors = 224267254,223888003)

eerhardt · 2018-10-10T14:28:40Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

+            /// <paramref name="toCompare"/> objects are used for comparison only.
+            /// </summary>
+            /// <returns>Whether this test succeeded.</returns>
+         protected bool TestCore(RunContextBase ctx, string cmdName, string args, int digitsOfPrecision, params PathArgument[] toCompare)


(nit) a single space was added to the beginning of the method, messing up the alignment.
Can you also align the /// comments as well? #Closed

The /// comments are still not aligned.

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

eerhardt · 2018-10-10T14:29:56Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

@@ -355,7 +360,14 @@ protected bool CheckTestOutputMatchesTrainTest(string trainTestOutPath, string t
    /// </summary>
    public abstract partial class TestDmCommandBase : TestCommandBase
    {
-        private bool TestCoreCore(RunContextBase ctx, string cmdName, string dataPath, PathArgument.Usage situation, OutputPath inModelPath, OutputPath outModelPath, string loaderArgs, string extraArgs, params PathArgument[] toCompare)
+        private bool TestCoreCore(RunContextBase ctx, string cmdName, string dataPath, PathArgument.Usage situation,
+            OutputPath inModelPath, OutputPath outModelPath, string loaderArgs, string extraArgs, params PathArgument[] toCompare){


(nit) opening curly brace should go on a new line. #Resolved

eerhardt · 2018-10-10T14:30:51Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

@@ -407,6 +419,11 @@ protected bool TestCore(RunContextBase ctx, string cmdName, string dataPath, str
            return TestCoreCore(ctx, cmdName, dataPath, PathArgument.Usage.DataModel, null, ctx.ModelPath(), loaderArgs, extraArgs, toCompare);
        }

+        protected bool TestCore(RunContextBase ctx, string cmdName, string dataPath, string loaderArgs, string extraArgs, int digitsOfPrecision, params PathArgument[] toCompare)


digitsOfPrecision [](start = 126, length = 17)

I don't see digitsOfPrecision being used in the method. Am I missing something? #Resolved

eerhardt

wschin · 2018-10-10T22:47:26Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

@@ -1944,8 +1944,10 @@ public void BinaryClassifierFieldAwareFactorizationMachineTest()

            // see https://github.com/dotnet/machinelearning/issues/404
            // in Linux, the clang sqrt() results vary highly from the ones in mac and Windows. 
+            // goign for 3 digits of precision, because the range of search is  (-0.0001 - 0.0001)


goign? #Resolved

wschin · 2018-10-10T22:49:32Z

test/Microsoft.ML.Predictor.Tests/TestPredictors.cs

@@ -1944,8 +1944,10 @@ public void BinaryClassifierFieldAwareFactorizationMachineTest()

            // see https://github.com/dotnet/machinelearning/issues/404
            // in Linux, the clang sqrt() results vary highly from the ones in mac and Windows. 
+            // goign for 3 digits of precision, because the range of search is  (-0.0001 - 0.0001)
+            // for one of the values, and the actual value is 0.00099999999999989


0.00099999999999989 is close enough to 0.001; it looks like the difference is smaller than 10^-7. Why do we need a larger tolerance? #Resolved

Yeah, but it is expecting a number in the -0.0001 to 0.0001 range. Still looking into as the test artifacts are not saved.

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

…as treating floats with scientific notation as strings; amending the regex to pick those up. Our custom Round method was rounding oen digit short than the digitsOfPrecision. Seems like Math.Round is not doing a bad job for the subset of tests i run. the delta calculated in the basetestbaseline, for some cases were passing the allowedVariance by a small fraction, outside of the digits we care to compare. Rounding that to truncate those digits before submitting it to the range comparison. Removing the digitsOfPrecision for a test drive on the CI, from some of the FFM tests, as it seems like they are doing ok without it for my local windows/linux runs.

tannergooding · 2018-10-11T22:57:10Z

test/Microsoft.ML.TestFramework/BaseTestBaseline.cs

@@ -521,26 +521,13 @@ private static void MatchNumberWithTolerance(MatchCollection firstCollection, Ma
                double f2 = double.Parse(secondCollection[i].ToString());

                double allowedVariance = Math.Pow(10, -digitsOfPrecision);
-                double delta = Round(f1, digitsOfPrecision) - Round(f2, digitsOfPrecision);
+                double delta = Math.Round(f1, digitsOfPrecision) - Math.Round(f2, digitsOfPrecision);


You should not be using Math.Round, it does not correctly handle significant digits that land on the left side of the decimal point (the integer part of the number). It only deals with the fractional half. #Resolved

re-introduced Round, thanks for the feedback. I moved to using it only on the difference, as rounding was not helping on a few cases.

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

This would be a good comment in the code - why we have a Round method.

In reply to: 224671986 [](ancestors = 224671986,224629061)

Zruty0

…the comparison to fail for certain cases.

eerhardt · 2018-10-12T17:36:12Z

test/Microsoft.ML.TestFramework/BaseTestPredictorsMaml.cs

@@ -354,7 +354,7 @@ public string GetLoaderTransformSettings(TestDataset dataset)
            string[] extraSettings = null, string extraTag = "", bool summary = false, int digitsOfPrecision = DigitsOfPrecision)
        {
            Contracts.Assert(IsActive);
-            Run_TrainTest(predictor, dataset, extraSettings, extraTag, summary: summary, digitsOfPrecision: digitsOfPrecision);
+           // Run_TrainTest(predictor, dataset, extraSettings, extraTag, summary: summary, digitsOfPrecision: digitsOfPrecision);


Was this a mistake? #Resolved

thanks for this, commented out while debugging.

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

eerhardt · 2018-10-12T17:37:24Z

test/Microsoft.ML.TestFramework/TestCommandBase.cs

            }
            return all;
-        }
+       }


(nit) you removed a space here. #Closed

sfilipi · 2018-10-13T05:24:18Z

This might resolve the fluctuations in precision for the MulticlassTreefeaturizedLR, and therefore not need to disable it like in #1185 .

ErcinDedeoglu · 2019-08-11T07:54:02Z

@sfilipi @eerhardt Could you give an example, how can I train decimal datas and predict a decimal data please?

eerhardt · 2019-08-12T02:39:22Z

@ErcinDedeoglu - you mean the C# ‘decimal’ type? ML.NET doesn’t support that type. You would need to convert to a ‘float’, run it through ML.NET, and then convert it back to a decimal.

ErcinDedeoglu · 2019-08-13T17:15:24Z

Dear @eerhardt,
When I cast my decimal value to float (float) i lost sensitivity...
decimal x = 2031630.73022778M;
float y = 2031630.75;
But the numbers after the point are also very important.

What's your suggestion?,
Thanks.

eerhardt · 2019-08-13T18:06:23Z

Yes, that is the downfall of using floating point numbers over a number that has a precise representation.

In the machine learning world, usually the precision is not that important. I don't know of anything in the industry that uses a precise number representation. Everything I've seen uses floating point numbers.

ErcinDedeoglu · 2019-08-13T20:44:59Z

@eerhardt Stock market uses :)

Enabling FFM tests

d37826f

sfilipi added the test related to tests label Oct 9, 2018

sfilipi self-assigned this Oct 9, 2018

sfilipi requested review from Ivanidzo4ka, wschin, eerhardt and Zruty0 October 9, 2018 22:17

wschin reviewed Oct 9, 2018

View reviewed changes

sfilipi added 3 commits October 9, 2018 22:43

adding baselines with different field in the missing values.

e5f2c16

Merge branch 'master' into ffmTests

d464b2e

moving baselines to the Common folder.

ae245f6

eerhardt reviewed Oct 10, 2018

View reviewed changes

eerhardt approved these changes Oct 10, 2018

View reviewed changes

sfilipi added 2 commits October 10, 2018 11:27

PR comments

8bceca4

Adjusting precision

3155949

wschin reviewed Oct 10, 2018

View reviewed changes

sfilipi added 3 commits October 10, 2018 15:56

Typo

c690c49

Capitals

6da2cb7

sfilipi requested a review from tannergooding October 11, 2018 22:51

tannergooding reviewed Oct 11, 2018

View reviewed changes

Zruty0 approved these changes Oct 12, 2018

View reviewed changes

sfilipi changed the title ~~Enabling FFM tests~~ WIP: Enabling FFM tests Oct 12, 2018

sfilipi added 3 commits October 11, 2018 22:08

substracting then rounding, as vice-versa is rounding up and causing …

899d93a

…the comparison to fail for certain cases.

trying to bullet-proof the comparison further

86d9a7b

further tweeking precision

425a99e

sfilipi changed the title ~~WIP: Enabling FFM tests~~ Enabling FFM tests Oct 12, 2018

eerhardt reviewed Oct 12, 2018

View reviewed changes

sfilipi added 2 commits October 12, 2018 20:31

spacing, uncommenting TrainTest method.

c1e9c43

lowering precision to 4

ba5fa18

sfilipi merged commit 2983312 into dotnet:master Oct 13, 2018

sfilipi deleted the ffmTests branch October 13, 2018 05:22

This was referenced Oct 13, 2018

skipping the MulticlassTreeFeaturizedLRTest on osx debug #1185

Closed

Fluctuation in calculations precision #404

Closed

frank-dong-ms-zz mentioned this pull request May 16, 2020

Legacy tests - partially disabled tests #5139

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling FFM tests #1206

Enabling FFM tests #1206

sfilipi commented Oct 9, 2018 •

edited

Loading

wschin Oct 9, 2018 •

edited by sfilipi

Loading

sfilipi Oct 10, 2018

sfilipi Oct 11, 2018

eerhardt Oct 10, 2018 •

edited

Loading

eerhardt Oct 12, 2018

eerhardt Oct 10, 2018 •

edited by sfilipi

Loading

eerhardt Oct 10, 2018 •

edited by sfilipi

Loading

eerhardt left a comment

wschin Oct 10, 2018 •

edited by sfilipi

Loading

wschin Oct 10, 2018 •

edited by sfilipi

Loading

sfilipi Oct 10, 2018

tannergooding Oct 11, 2018 •

edited by sfilipi

Loading

sfilipi Oct 12, 2018

eerhardt Oct 12, 2018

Zruty0 left a comment

eerhardt Oct 12, 2018 •

edited by sfilipi

Loading

sfilipi Oct 13, 2018

eerhardt Oct 12, 2018 •

edited

Loading

sfilipi commented Oct 13, 2018

ErcinDedeoglu commented Aug 11, 2019

eerhardt commented Aug 12, 2019

ErcinDedeoglu commented Aug 13, 2019

eerhardt commented Aug 13, 2019

ErcinDedeoglu commented Aug 13, 2019

Enabling FFM tests #1206

Enabling FFM tests #1206

Conversation

sfilipi commented Oct 9, 2018 • edited Loading

wschin Oct 9, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Oct 10, 2018

Choose a reason for hiding this comment

sfilipi Oct 11, 2018

Choose a reason for hiding this comment

eerhardt Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt Oct 12, 2018

Choose a reason for hiding this comment

eerhardt Oct 10, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

eerhardt Oct 10, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

wschin Oct 10, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

wschin Oct 10, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Oct 10, 2018

Choose a reason for hiding this comment

tannergooding Oct 11, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Oct 12, 2018

Choose a reason for hiding this comment

eerhardt Oct 12, 2018

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

eerhardt Oct 12, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Oct 13, 2018

Choose a reason for hiding this comment

eerhardt Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi commented Oct 13, 2018

ErcinDedeoglu commented Aug 11, 2019

eerhardt commented Aug 12, 2019

ErcinDedeoglu commented Aug 13, 2019

eerhardt commented Aug 13, 2019

ErcinDedeoglu commented Aug 13, 2019

sfilipi commented Oct 9, 2018 •

edited

Loading

wschin Oct 9, 2018 •

edited by sfilipi

Loading

eerhardt Oct 10, 2018 •

edited

Loading

eerhardt Oct 10, 2018 •

edited by sfilipi

Loading

eerhardt Oct 10, 2018 •

edited by sfilipi

Loading

wschin Oct 10, 2018 •

edited by sfilipi

Loading

wschin Oct 10, 2018 •

edited by sfilipi

Loading

tannergooding Oct 11, 2018 •

edited by sfilipi

Loading

eerhardt Oct 12, 2018 •

edited by sfilipi

Loading

eerhardt Oct 12, 2018 •

edited

Loading