Update tests (using Iris dataset) to use new API #2008

abgoswam · 2019-01-03T19:30:27Z

Update a couple of tests (using Iris dataset) to use the new API.

codemzs · 2019-01-03T19:42:50Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationTests.cs

    public partial class ScenariosTests
    {
        [Fact]
-        public void TrainAndPredictIrisModelTest()
+        public void New_TrainAndPredictIrisModelTest()


New_ [](start = 20, length = 4)

I believe "New" prefix was used because we had two APIs but now the old API is gone and there is just one API, do you feel we still need to prefix test with "New"? #Resolved

I added it during development when I had both tests in this file. Forgot yo drop the 'New_' prefix after deleting the old test. Will fix.

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

fixed

In reply to: 245113327 [](ancestors = 245113327,245111276)

codemzs · 2019-01-03T19:45:12Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationTests.cs

+
+            var pipe = mlContext.Transforms.Concatenate("Features", "SepalLength", "SepalWidth", "PetalLength", "PetalWidth")
+                            .Append(mlContext.Transforms.Normalize("Features"))
+                            .AppendCacheCheckpoint(mlContext)


Why are we adding normalizer transform? just curious. #Resolved

I think in the new API the paradigm is to be explicit about whether are using Normalizers in the pipeline .

(In the old Legacy API normalizers used to be auto-inserted based on the algorithm)

Hence decided to be explicit about normalizers. Please see #1604 which talks about removing 'smarts' (i.e. no auto-normalization, no auto-caching and no auto-calibration) in the new API

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

codemzs · 2019-01-03T19:52:33Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationTests.cs

-            Assert.Equal(1, matrix[2, 1]);
-            Assert.Equal(1, matrix["2", "1"]);
-            Assert.Equal(49, matrix[2, 2]);
-            Assert.Equal(49, matrix["2", "2"]);


Do we not have confusion matrix in metrics? #Resolved

I do not see it inside metrics. Who would be the best person to confirm whether we support ConfusionMatrix in the new API ?

If we do, is there an example I can follow ? I do not see it in the CookBook either

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

Created issue #2009 to bring back support for ConfusionMatrix in the new API

In reply to: 245117612 [](ancestors = 245117612,245114114)

codemzs · 2019-01-03T19:53:00Z

test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj

@@ -22,7 +22,6 @@
    <ProjectReference Include="..\..\src\Microsoft.ML.StaticPipe\Microsoft.ML.StaticPipe.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.TensorFlow.StaticPipe\Microsoft.ML.TensorFlow.StaticPipe.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.TensorFlow\Microsoft.ML.TensorFlow.csproj" />
-    <ProjectReference Include="..\..\src\Microsoft.ML.Legacy\Microsoft.ML.Legacy.csproj" />


[](start = 4, length = 87)

Nice!! #Resolved

Ivanidzo4ka · 2019-01-03T20:15:13Z

@abgoswam Can we just delete this files instead of modifying them? We have test which do evaluation, we have tests which do predictions (with new API), what's the point of having this ones? #Resolved

codemzs · 2019-01-03T20:39:58Z

@Ivanidzo4ka We did that and it results in code coverage drop. Check out my PR that removes these files. It’s possible removing some tests might be ok but which ones will require tests by tests code coverage analysis. It might just be easier to convert all the test to new API. Even though these tests are doing training, evaluation, etc but they may be using different transforms, trainers, settings, etc. #Resolved

wschin · 2019-01-03T21:06:17Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationTests.cs


            Assert.Equal(.98, metrics.AccuracyMacro);
            Assert.Equal(.98, metrics.AccuracyMicro, 2);
-            Assert.Equal(.06, metrics.LogLoss, 2);
+            Assert.InRange(metrics.LogLoss, .05, .06);


Could we use Assert.Equal(…, …, 2)? #Resolved

I will also prefer Equal #Resolved

fixed

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

wschin · 2019-01-03T21:07:40Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationWithStringLabelTests.cs


            Assert.Equal(.98, metrics.AccuracyMacro);
            Assert.Equal(.98, metrics.AccuracyMicro, 2);
-            Assert.Equal(.06, metrics.LogLoss, 2);
+            Assert.InRange(metrics.LogLoss, .05, .06);


Assert.Equal(metrics.Logloss, 0.055, 3) maybe? #Resolved

fixed

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

wschin · 2019-01-03T21:08:14Z

test/Microsoft.ML.Tests/Scenarios/IrisPlantClassificationWithStringLabelTests.cs

            public string IrisPlantType;
        }
+
+        public class IrisPredictionWithStringLabel


Are those data structures used outside this file? #Resolved

Made it private. To make the compiler happy I am using properties instead of fields

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

Ivanidzo4ka · 2019-01-03T21:12:07Z

I've run it with tests and without tests, and only dll for which code coverage went down is Legacy, which is expected. Everything else either state same, or went up by 0.01% :)

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

codemzs · 2019-01-03T21:18:16Z

I have explained to you in person the issue with running code coverage per test in the context of parallelizing test conversion effort. Say you have two tests, each assigned to two devs, these two tests are exercising same code paths, if both devs in parallel run code coverage for the test assigned to them then both will end up removing both tests because of this race condition and then we lose code coverage. I do not see this as a safe and quick way to prevent regression in code coverage due to the removal Legacy API binary. The fastest and safest way is to convert all the test to the new API. Correct me if I'm wrong. Also if you are comparing against master (with test) you will need to remove legacy dll from the code coverage analysis so we don't see code coverage going up just because we removed a test!

In reply to: 451278660 [](ancestors = 451278660,451270185)

wschin

codemzs

Update Iris tests to use new API

b38dac2

abgoswam requested review from codemzs, artidoro and wschin January 3, 2019 19:30

codemzs reviewed Jan 3, 2019

View reviewed changes

wschin reviewed Jan 3, 2019

View reviewed changes

abgoswam added 2 commits January 3, 2019 21:31

review comments

b38109d

review comments -2 . made couple of classes private

9418f5d

wschin approved these changes Jan 3, 2019

View reviewed changes

codemzs approved these changes Jan 3, 2019

View reviewed changes

removed dummy initialization, and using properties instead

f1a05aa

abgoswam merged commit 1d864c4 into dotnet:master Jan 3, 2019

abgoswam deleted the abgoswam/remove_legacy_tests branch January 13, 2019 18:11

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tests (using Iris dataset) to use new API #2008

Update tests (using Iris dataset) to use new API #2008

abgoswam commented Jan 3, 2019

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019

abgoswam Jan 3, 2019

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019

abgoswam Jan 3, 2019

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Jan 3, 2019 •

edited by abgoswam

Loading

codemzs commented Jan 3, 2019 •

edited by abgoswam

Loading

wschin Jan 3, 2019 •

edited by abgoswam

Loading

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019

wschin Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019

wschin Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019 •

edited

Loading

Ivanidzo4ka commented Jan 3, 2019

codemzs commented Jan 3, 2019 •

edited

Loading

wschin left a comment

codemzs left a comment

Update tests (using Iris dataset) to use new API #2008

Update tests (using Iris dataset) to use new API #2008

Conversation

abgoswam commented Jan 3, 2019

codemzs Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

codemzs Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

codemzs Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

codemzs Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Jan 3, 2019 • edited by abgoswam Loading

codemzs commented Jan 3, 2019 • edited by abgoswam Loading

wschin Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

codemzs Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

wschin Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019

Choose a reason for hiding this comment

wschin Jan 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Jan 3, 2019

codemzs commented Jan 3, 2019 • edited Loading

wschin left a comment

Choose a reason for hiding this comment

codemzs left a comment

Choose a reason for hiding this comment

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Jan 3, 2019 •

edited by abgoswam

Loading

codemzs commented Jan 3, 2019 •

edited by abgoswam

Loading

wschin Jan 3, 2019 •

edited by abgoswam

Loading

codemzs Jan 3, 2019 •

edited by abgoswam

Loading

wschin Jan 3, 2019 •

edited by abgoswam

Loading

wschin Jan 3, 2019 •

edited by abgoswam

Loading

abgoswam Jan 3, 2019 •

edited

Loading

codemzs commented Jan 3, 2019 •

edited

Loading