Creation of components through MLContext and cleanup (Onnx, Tensorflow, SelectColumn, KeytoBinVec, ValueMap) #2367

artidoro · 2019-02-01T14:08:42Z

This PR is part of the work outlined in #1798, and focuses on the GcnNorm, LpNorm, RandomFourier, CustomStopWords, VectorWhiten, PCA transformers/estimators:

Internalize constructors of estimators and transformers
Keep ColumnInfo and move it to to the estimators The ColumnInfo structure should live in the estimators, rather than transformers #1760
Rename Arguments -> Options
Internalize Options only when they are not used by public constructor. For all other cases, retain Options as public Arguments class should be made internal when possible #1758

This PR is marked as WIP because I have to figure out how to make the DNN assemblies BestFriends with the ONNX assembly. I need to figure out how to do that.

codecov · 2019-02-04T20:06:00Z

Codecov Report

Merging #2367 into master will decrease coverage by 0.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #2367      +/-   ##
==========================================
- Coverage   71.22%   71.21%   -0.02%     
==========================================
  Files         785      785              
  Lines      140977   140954      -23     
  Branches    16116    16113       -3     
==========================================
- Hits       100412   100379      -33     
- Misses      36096    36109      +13     
+ Partials     4469     4466       -3

Flag	Coverage Δ
#Debug	`71.21% <80%> (-0.02%)`	⬇️
#production	`67.57% <78.42%> (-0.02%)`	⬇️
#test	`85.28% <85%> (-0.01%)`	⬇️

src/Microsoft.ML.Data/Commands/SaveDataCommand.cs

src/Microsoft.ML.Data/Transforms/ColumnSelecting.cs

src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs

src/Microsoft.ML.OnnxTransform/Microsoft.ML.OnnxTransform.csproj

src/Microsoft.ML.OnnxTransform/OnnxCatalog.cs

src/Microsoft.ML.OnnxTransform/OnnxTransform.cs

src/Microsoft.ML.Transforms/KeyToVectorMapping.cs

singlis

singlis · 2019-02-05T02:36:07Z

overall it looks good to me - I left comments about the CreateKeep/CreateDrop #Resolved

artidoro · 2019-02-05T02:39:35Z

Thank you very much for reviewing @singlis! I will address your comments. #Resolved

Ivanidzo4ka · 2019-02-06T04:51:21Z

src/Microsoft.ML.Transforms/KeyToVectorMapping.cs

@@ -29,38 +29,18 @@

 namespace Microsoft.ML.Transforms.Conversions
 {
+    /// <summary>
+    ///  Converts the key types back to binary verctors.


verctors [](start = 47, length = 8)

verctors [](start = 47, length = 8)

typeo #Closed

Ivanidzo4ka · 2019-02-06T04:56:06Z

src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs

+        /// <example>
+        /// <format type="text/markdown">
+        /// <![CDATA[
+        /// [!code-csharp[ScoreTensorFlowModel](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/TensorFlowTransform.cs)]


TensorFlowTransform [](start = 108, length = 19)

TensorFlowTransform [](start = 108, length = 19)

Zeeshan A is trying to rename this file, so be careful
#2429 #Closed

please merge with master
#2429 got merge and this file got moved.

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

Just rebased again, should be good now.

In reply to: 254514215 [](ancestors = 254514215,254137918)

Did you push your changes? because I still see Dynamic/TensorFlowTransform.cs instead of TensorFlow/ImageClassification.cs

In reply to: 254515347 [](ancestors = 254515347,254514215,254137918)

I am sorry I think I did not rebase earlier. I now rebased and pushed again and I see Zeeshan's change now. #Resolved

I still see in line 24 link to /// [!code-csharp[ScoreTensorFlowModel](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/TensorFlowTransform.cs)]

And can you please stop using your rebase magic? Codeflow continue to stay on same iteration, and it get's hard to track which files get changed and which are not?

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

Ah now I understood what you meant! the links will be broken sorry!! Fixing right away #Closed

Ok now I think I did what you were saying from the beginning, I didn't understand you were talking about the link. Just pushed a new iteration.

Ivanidzo4ka · 2019-02-06T04:56:31Z

src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs

+        /// <param name="catalog">The transform's catalog.</param>
+        /// <param name="modelLocation">Location of the TensorFlow model.</param>
+        /// <param name="inputColumnName"> The name of the model inputs.</param>
+        /// <param name="outputColumnName">The name of the requested model outputs.</param>


last word in each sentence is plural. should be singular. input/output instead of inputs/outputs. #Resolved

Ivanidzo4ka · 2019-02-06T04:58:47Z

src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs

+        /// Scores a dataset using a pre-traiend TensorFlow model specified via <paramref name="tensorFlowModel"/>.
+        /// </summary>
+        /// <param name="catalog">The transform's catalog.</param>
+        /// <param name="tensorFlowModel">The pre-trained TensorFlow model.</param>


The pre-trained TensorFlow model. [](start = 42, length = 33)

The pre-trained TensorFlow model. [](start = 42, length = 33)

I would call it pre-loaded instead of pre-trained. They all pre-trained I believe, but this one you load manually in memory, and for other you just pass path to model. #Closed

artidoro · 2019-02-06T22:19:01Z

/// The SelectColumns Transforms allows the user to specify columns to drop or keep from a given input.

Good point!

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

Refers to: src/Microsoft.ML.Data/Transforms/ColumnSelecting.cs:125 in 1fd3ec2. [](commit_id = 1fd3ec2, deletion_comment = False)

artidoro · 2019-02-06T22:22:18Z

public static class SkipFilter

Yes you are right! Thank you :)

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

Refers to: src/Microsoft.ML.Data/Transforms/SkipTakeFilter.cs:269 in 1fd3ec2. [](commit_id = 1fd3ec2, deletion_comment = False)

artidoro · 2019-02-07T00:26:25Z

    public static ValueMappingEstimator<TInputType, TOutputType> ValueMap<TInputType, TOutputType>(

I would open an issue about it and discuss there if you want to change it

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

Refers to: src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs:172 in 1fd3ec2. [](commit_id = 1fd3ec2, deletion_comment = False)

zeahmed · 2019-02-07T05:37:25Z

src/Microsoft.ML.Data/Transforms/ColumnSelecting.cs

@@ -48,7 +55,7 @@ public sealed class ColumnSelectingEstimator : TrivialEstimator<ColumnSelectingT
        /// <param name="env">Instance of the host environment.</param>
        /// <param name="keepColumns">The array of column names to keep.</param>
        private ColumnSelectingEstimator(IHostEnvironment env, params string[] keepColumns)


private [](start = 8, length = 7)

I don't see any use of this constructor except for being used at line 90. Since you are cleaning up the code as well, I would suggest removing it. #Resolved

Good point! I removed that

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

artidoro · 2019-02-07T06:45:55Z

src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs

+        /// <param name="tensorFlowModel">The pre-loaded TensorFlow model.</param>
+        /// <param name="inputColumnName"> The name of the model input.</param>
+        /// <param name="outputColumnName">The name of the requested model output.</param>
+        public static TensorFlowEstimator ScoreTensorFlowModel(this TransformsCatalog catalog,


Adding the example that was linked below.

Ivanidzo4ka · 2019-02-07T17:40:20Z

src/Microsoft.ML.OnnxTransform/OnnxCatalog.cs

+        /// Applies a pre-trained Onnx model.
+        /// All column names are provided, the input data column names/types must exactly match
+        /// all model input names. All possible output columns are then generated, with names/types
+        /// specified by the model.
        /// </summary>


you can put this one in <remark> section #Resolved

Ivanidzo4ka · 2019-02-07T17:41:31Z

src/Microsoft.ML.OnnxTransform/OnnxCatalog.cs

-            => new OnnxScoringEstimator(CatalogUtils.GetEnvironment(catalog), transformer);
+        /// <param name="modelFile">The path of the file containing the ONNX model.</param>
+        /// <param name="outputColumnName">The input column.</param>
+        /// <param name="inputColumnName">The output column resulting from the transformation.</param>


can you change their description? you put input column for outputColumnName and vice versa. #Resolved

Oh that was bad thank you!

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

Ivanidzo4ka · 2019-02-07T17:41:46Z

src/Microsoft.ML.OnnxTransform/OnnxCatalog.cs

+        /// <param name="catalog">The transform's catalog.</param>
+        /// <param name="modelFile">The path of the file containing the ONNX model.</param>
+        /// <param name="outputColumnNames">The input columns.</param>
+        /// <param name="inputColumnNames">The output columns resulting from the transformation.</param>


can you change their description? you put input columns for outputColumnNames and vice versa. #Resolved

Ivanidzo4ka

artidoro changed the title ~~Creation of components through MLContext and cleanup (Onnx, Tensorflow, SelectColumn, KeytoBinVec, ValueMap)~~ WIP: Creation of components through MLContext and cleanup (Onnx, Tensorflow, SelectColumn, KeytoBinVec, ValueMap) Feb 1, 2019

artidoro mentioned this pull request Feb 1, 2019

Creation of components through MLContext: advanced options and other feedback #1798

Closed

artidoro self-assigned this Feb 1, 2019

artidoro added the API Issues pertaining the friendly API label Feb 1, 2019

artidoro requested review from Ivanidzo4ka, sfilipi and zeahmed February 3, 2019 03:17

artidoro changed the title ~~WIP: Creation of components through MLContext and cleanup (Onnx, Tensorflow, SelectColumn, KeytoBinVec, ValueMap)~~ Creation of components through MLContext and cleanup (Onnx, Tensorflow, SelectColumn, KeytoBinVec, ValueMap) Feb 3, 2019

artidoro mentioned this pull request Feb 3, 2019

Fix naming of Options argument in TensorFlowTransform API #2392

Merged

artidoro force-pushed the round5 branch 2 times, most recently from b1dfe49 to 50505e3 Compare February 3, 2019 22:21

artidoro mentioned this pull request Feb 4, 2019

Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

Merged

Ivanidzo4ka reviewed Feb 4, 2019

View reviewed changes

src/Microsoft.ML.Data/Commands/SaveDataCommand.cs Outdated Show resolved Hide resolved