Make ImageClassification API an ITrainerEstimator and refactor code. #4372

codemzs · 2019-10-24T10:04:15Z

In addition to converting ImageClassification API to ITrainerEstimator it also fixes #4276

codecov · 2019-10-24T17:10:35Z

Codecov Report

Merging #4372 into master will increase coverage by <.01%.
The diff coverage is 89.09%.

@@            Coverage Diff             @@
##           master    #4372      +/-   ##
==========================================
+ Coverage   74.63%   74.64%   +<.01%     
==========================================
  Files         883      883              
  Lines      155117   155036      -81     
  Branches    16931    16911      -20     
==========================================
- Hits       115768   115723      -45     
+ Misses      34601    34569      -32     
+ Partials     4748     4744       -4

Flag	Coverage Δ
#Debug	`74.64% <89.09%> (ø)`	⬆️
#production	`70.2% <82.85%> (+0.01%)`	⬆️
#test	`89.56% <100%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
src/Microsoft.ML.Dnn/ImageClassificationTrainer.cs	`93.12% <ø> (ø)`
src/Microsoft.ML.Dnn/DnnUtils.cs	`69.92% <100%> (ø)`	⬆️
...rosoft.ML.Data/Scorers/PredictedLabelScorerBase.cs	`81.71% <100%> (ø)`	⬆️
...cenariosWithDirectInstantiation/TensorflowTests.cs	`90.87% <100%> (-0.02%)`	⬇️
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs	`85.78% <100%> (ø)`	⬆️
....ML.Data/Scorers/MulticlassClassificationScorer.cs	`60.39% <100%> (+0.26%)`	⬆️
src/Microsoft.ML.Dnn/DnnCatalog.cs	`100% <100%> (ø)`	⬆️
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs	`91.05% <66.66%> (-1.08%)`	⬇️
test/Microsoft.ML.AutoML.Tests/DatasetUtil.cs	`79.06% <0%> (-6.98%)`	⬇️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs	`85.15% <0%> (-1.75%)`	⬇️
... and 5 more
#Resolved

…to imgclassificationtrainerest

eerhardt · 2019-10-24T18:52:24Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

-                PredictedLabelColumnName = predictedLabelColumnName,
-                ValidationSet = validationSet
-            };
+            options.EarlyStoppingCriteria = options.DisableEarlyStopping ? null : options.EarlyStoppingCriteria ?? new ImageClassificationTrainer.EarlyStopping();


I made a comment on the previous PR that you said you would address in this PR.

#4362 (comment) #Closed

eerhardt · 2019-10-24T18:52:58Z

src/Microsoft.ML.Dnn/DnnCatalog.cs

+            string featureColumnName = DefaultColumnNames.Features,
+            string scoreColumnName = DefaultColumnNames.Score,
+            string predictedLabelColumnName = DefaultColumnNames.PredictedLabel,
+            IDataView validationSet = null)


I thought you were going to move the validationSet to be during the Fit method? #Resolved

I have and it works well when the trainer is the last item in the pipeline but if it is not then the pipeline becomes nasty and you will need to save the model twice. Most people would want to add KeyToValue Transform after this trainer to convert predictedLabel to string type for example .

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

eerhardt · 2019-10-24T18:55:24Z

public class ImageClassificationTrainer :

public sealed class ? #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:68 in 9a01968. [](commit_id = 9a01968, deletion_comment = False)

eerhardt · 2019-10-24T18:56:25Z

    public delegate void ImageClassificationMetricsCallback(ImageClassificationMetrics metrics);

Do we need a new delegate here? Or can we instead use Action<ImageClassificationMetrics> ? #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:124 in 9a01968. [](commit_id = 9a01968, deletion_comment = False)

eerhardt · 2019-10-24T18:58:02Z

    public sealed class Options : TrainerInputBaseWithLabel

(nit) xml doc comments on the class. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:336 in 9a01968. [](commit_id = 9a01968, deletion_comment = False)

eerhardt · 2019-10-24T18:59:05Z

        public IDataView ValidationSet;

Same comment here about the ValidationSet being passed into Fit, like the other algorithms in ML.NET. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:426 in 9a01968. [](commit_id = 9a01968, deletion_comment = False)

…to imgclassificationtrainerest

codemzs · 2019-10-25T18:31:40Z

        public IDataView ValidationSet;

see my previous comment. We have an api for Fit that takes both train and validation set.

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

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:426 in 9a01968. [](commit_id = 9a01968, deletion_comment = False)

src/Microsoft.ML.Dnn/ImageClassificationTrainer.cs

src/Microsoft.ML.Dnn/DnnUtils.cs

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs

eerhardt · 2019-10-25T19:08:17Z

};

(nit) unnecessary semi-colon. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:311 in 72e0eee. [](commit_id = 72e0eee, deletion_comment = False)

eerhardt · 2019-10-25T19:12:30Z

            new Span<float>(classifier.ClassProbabilities, 0, _classCount).CopyTo(editor.Values);

I think this should be changed to pass the editor.Values into the classifier.Score method. Then we only need to copy the values a single time - from TensorFlow's native memory directly into the VBuffer.

The way we have it today, we are holding this extra float[] _classProbabilities as an intermediate step, which isn't necessary. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1427 in 72e0eee. [](commit_id = 72e0eee, deletion_comment = False)

eerhardt · 2019-10-25T19:13:36Z

        public void Score(VBuffer<byte> image)

(nit) should be in VBuffer<byte> image. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1407 in 72e0eee. [](commit_id = 72e0eee, deletion_comment = False)

…to imgclassificationtrainerest

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs

eerhardt · 2019-10-25T21:07:19Z

public sealed class ImageClassificationModelParameters : ModelParametersBase<VBuffer<float>>, IValueMapper

xml docs on public types. #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1281 in ea79b6f. [](commit_id = ea79b6f, deletion_comment = False)

eerhardt · 2019-10-25T21:08:24Z

    public DataViewType InputType => _inputType;

Do InputType and OutputType need to be public? #Resolved

Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1328 in ea79b6f. [](commit_id = ea79b6f, deletion_comment = False)

eerhardt

LGTM

…otnet#4372) * Make ImageClassification API an ITrainerEstimator and refactor code. * comments. * comments. * Fix premature session close. * PR feedback. * PR feedback. * PR feedback. * PR feedback. * PR feedback. * PR feedback. * PR feedback. * PR feedback. * PR feedback.

Make ImageClassification API an ITrainerEstimator and refactor code.

121bd14

codemzs requested a review from a team as a code owner October 24, 2019 10:04

codemzs requested review from yaeldekel, eerhardt, bpstark, harshithapv and ashbhandare October 24, 2019 10:04

comments.

8e96698

codemzs requested a review from CESARDELATORRE October 24, 2019 10:11

comments.

9a01968

codemzs added 2 commits October 24, 2019 11:36

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

436fb8c

…to imgclassificationtrainerest

Fix premature session close.

be0b85d

eerhardt reviewed Oct 24, 2019

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

a43dee1

…to imgclassificationtrainerest

PR feedback.

72e0eee

eerhardt reviewed Oct 25, 2019

View reviewed changes

src/Microsoft.ML.Dnn/ImageClassificationTrainer.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 25, 2019

View reviewed changes

src/Microsoft.ML.Dnn/DnnUtils.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 25, 2019

View reviewed changes

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 25, 2019

View reviewed changes

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs Outdated Show resolved Hide resolved

codemzs mentioned this pull request Oct 25, 2019

Unit-test for older model versions that use multiclass scorer #4389

Open

codemzs added 3 commits October 25, 2019 13:41

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

9658a14

…to imgclassificationtrainerest

PR feedback.

f383b9d

PR feedback.

3644e89

eerhardt reviewed Oct 25, 2019

View reviewed changes

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs Outdated Show resolved Hide resolved

codemzs added 4 commits October 25, 2019 13:50

PR feedback.

564234f

PR feedback.

ea79b6f

PR feedback.

4ade168

PR feedback.

b7a7ff2

codemzs added 2 commits October 25, 2019 14:08

PR feedback.

4a218e7

PR feedback.

2639790

eerhardt approved these changes Oct 25, 2019

View reviewed changes

codemzs merged commit b1788c3 into dotnet:master Oct 25, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ImageClassification API an ITrainerEstimator and refactor code. #4372

Make ImageClassification API an ITrainerEstimator and refactor code. #4372

codemzs commented Oct 24, 2019

codecov bot commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt Oct 24, 2019 •

edited

Loading

eerhardt Oct 24, 2019 •

edited by codemzs

Loading

codemzs Oct 25, 2019

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

codemzs commented Oct 25, 2019

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt left a comment

Make ImageClassification API an ITrainerEstimator and refactor code. #4372

Make ImageClassification API an ITrainerEstimator and refactor code. #4372

Conversation

codemzs commented Oct 24, 2019

codecov bot commented Oct 24, 2019 • edited by codemzs Loading

Codecov Report

eerhardt Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Oct 24, 2019 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Oct 25, 2019

Choose a reason for hiding this comment

eerhardt commented Oct 24, 2019 • edited by codemzs Loading

eerhardt commented Oct 24, 2019 • edited by codemzs Loading

eerhardt commented Oct 24, 2019 • edited by codemzs Loading

eerhardt commented Oct 24, 2019 • edited by codemzs Loading

codemzs commented Oct 25, 2019

eerhardt commented Oct 25, 2019 • edited by codemzs Loading

eerhardt commented Oct 25, 2019 • edited by codemzs Loading

eerhardt commented Oct 25, 2019 • edited by codemzs Loading

eerhardt commented Oct 25, 2019 • edited by codemzs Loading

eerhardt commented Oct 25, 2019 • edited by codemzs Loading

eerhardt left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt Oct 24, 2019 •

edited

Loading

eerhardt Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 24, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading

eerhardt commented Oct 25, 2019 •

edited by codemzs

Loading