Clean SamplesUtils by wschin · Pull Request #3594 · dotnet/machinelearning

wschin · 2019-04-25T22:46:56Z

This PR cleans SampleUtils a bit. Toward #3584.

If a function/class is used only in less than two places, we copy them wherever it's used.
Function/class referenced by nothing is removed.

Currently, we can't not remove SamplesUtils entirely because some samples are still using it.

shmoradims · 2019-04-25T22:54:56Z

+            Console.WriteLine($"Mean Squared Error: {metrics.MeanSquaredError:F2}");
+            Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
+            Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
+        }


for trainers, we should modify the .ttinclude template file, not the cs file directly. #Resolved

*Advanced.cs are not API samples. They are legacy end-to-end pipelines saved for future use. #Resolved

shmoradims · 2019-04-25T23:07:13Z

+            data.Add(new SampleSentimentData { Sentiment = true, SentimentText = "Best game I've ever played." });
+            data.Add(new SampleSentimentData { Sentiment = false, SentimentText = "==RUDE== Dude, 2" });
+            data.Add(new SampleSentimentData { Sentiment = true, SentimentText = "Until the next game, this is the best Xbox game!" });
+            return data;


let's use initializer, instead of Add one-by-one, remove the function-call, and just move the list up in the Example().

var data = new List{
datapoint1,
datapoint2,
....
} #Resolved

shmoradims · 2019-04-25T23:10:18Z

+            data.Add(new SampleVectorOfNumbersData
+            {
+                Features = new float[10] { 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }
+            });


similar change. please create the data inline with initializers, similar to NgramExtraction.

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

najeeb-kazmi · 2019-04-26T00:08:41Z


            // Get a small dataset as an IEnumerable and convert to IDataView.
-            var data = Microsoft.ML.SamplesUtils.DatasetUtils.GetSentimentData();
+            var data = GetSentimentData();


var [](start = 12, length = 3)

You have IEnumerable<SampleSentimentData> in the NGram sample. Let's adopt a consistent scheme, either var everywhere or the concrete type everywhere.

Will do var.

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

najeeb-kazmi · 2019-04-26T00:11:12Z

    private static (int lines, double columnAverage, double elapsedSeconds) TimeToScanIDataView(MLContext mlContext, IDataView data)

I thought we had decided to not use tuples, at least in the API. Is it okay for samples? @shmoradims

Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs:42 in 70a3a18. [](commit_id = 70a3a18, deletion_comment = False)

najeeb-kazmi

wschin · 2019-04-26T15:46:15Z

    private static (int lines, double columnAverage, double elapsedSeconds) TimeToScanIDataView(MLContext mlContext, IDataView data)

I think they are fine. It's a part of C# language and this is a C# sample. I remember one reason not to use tuple is that F# doesn't support it well.

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

Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs:42 in 70a3a18. [](commit_id = 70a3a18, deletion_comment = False)

codecov · 2019-04-26T16:28:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@51b10fc). Click here to learn what that means.
The diff coverage is 97.5%.

@@            Coverage Diff            @@
##             master    #3594   +/-   ##
=========================================
  Coverage          ?    72.9%           
=========================================
  Files             ?      807           
  Lines             ?   145214           
  Branches          ?    16237           
=========================================
  Hits              ?   105867           
  Misses            ?    34924           
  Partials          ?     4423

Flag	Coverage Δ
#Debug	`72.9% <97.5%> (?)`
#production	`68.43% <0%> (?)`
#test	`89.05% <100%> (?)`

Impacted Files	Coverage Δ
...c/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs	`39.88% <0%> (ø)`
...rosoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs	`100% <100%> (ø)`

shmoradims · 2019-04-26T18:06:06Z

    private static (int lines, double columnAverage, double elapsedSeconds) TimeToScanIDataView(MLContext mlContext, IDataView data)

it's ok here. but since it's just a util, it would be cleaner to make the function void, and print the values in the function instead of returning the stats and printing in the main example. The user can still see the produced output, from the proceeding comments. So this is just simpler and we don't have to worry if users know about tuples or not.

In reply to: 487105281 [](ancestors = 487105281,486880743)

Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs:42 in 70a3a18. [](commit_id = 70a3a18, deletion_comment = False)

shmoradims

wschin requested review from Ivanidzo4ka, codemzs and shmoradims April 25, 2019 22:46

wschin self-assigned this Apr 25, 2019

shmoradims reviewed Apr 25, 2019

View reviewed changes

najeeb-kazmi reviewed Apr 26, 2019

View reviewed changes

najeeb-kazmi approved these changes Apr 26, 2019

View reviewed changes

shmoradims approved these changes May 14, 2019

View reviewed changes

wschin added 2 commits May 17, 2019 10:01

Clean SamplesUtils

f44d3f6

Address comments

936c406

wschin force-pushed the clean-samplesutils branch from 55b723d to 936c406 Compare May 17, 2019 17:01

Merge branch 'master' into clean-samplesutils

9da2456

wschin merged commit 4073653 into dotnet:master May 24, 2019

wschin deleted the clean-samplesutils branch May 24, 2019 20:20

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

Uh oh!

Conversation

wschin commented Apr 25, 2019

Uh oh!

shmoradims Apr 25, 2019 • 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!

wschin Apr 25, 2019 • edited by shmoradims Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Apr 25, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmoradims Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

shmoradims Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

najeeb-kazmi Apr 26, 2019 • 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!

wschin Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

najeeb-kazmi commented Apr 26, 2019 • edited by shmoradims Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

najeeb-kazmi left a comment

Choose a reason for hiding this comment

Uh oh!

wschin commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shmoradims commented Apr 26, 2019

Uh oh!

shmoradims left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shmoradims Apr 25, 2019 •

edited

Loading

wschin Apr 25, 2019 •

edited by shmoradims

Loading

shmoradims Apr 25, 2019 •

edited by wschin

Loading

najeeb-kazmi Apr 26, 2019 •

edited

Loading

najeeb-kazmi commented Apr 26, 2019 •

edited by shmoradims

Loading

wschin commented Apr 26, 2019 •

edited

Loading

codecov Bot commented Apr 26, 2019 •

edited

Loading