NAReplace estimator #917

Ivanidzo4ka · 2018-09-14T18:35:57Z

Converts NAReplace to estimator

Zruty0 · 2018-09-14T20:33:40Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

+            public readonly bool ImputeBySlot;
+            public readonly ReplacementKind Kind;
+
+            public ColumnInfo(string input, string output, ReplacementKind kind = ReplacementKind.DefaultValue, bool imputeBySlot = true)


ColumnInfo [](start = 19, length = 10)

summary comment #Resolved

Is it a good comment?

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

summary: Describes how the transformer handles one column pair.
input: name of input column
output: name of output column
replacementMode: what to replace the missing value with
imputeBySlot: if true, per-slot imputation of replacement is performed. Otherwise, replacement value is imputed for the entire vector column. This setting is ignored for scalars and variable vectors, where imputation is always for the entire column.

In reply to: 217855806 [](ancestors = 217855806,217834730)

Zruty0 · 2018-09-14T20:34:13Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

+            return columns.Select(x => (x.Input, x.Output)).ToArray();
+        }
+
+        ///IVAN: move to mapper.


move to mapper. [](start = 17, length = 15)

maybe just turn into one 'type' ? The rest is accessible via _parent.ColumnPairs #Resolved

I'm actually quite tempted to move whole ColInfo into MapperBase.
Since it's heavily used almost in every mapper I wrote.

But yeah, I have _types array which I will use in Estimator.

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

Moving ColInfo to MapperBase will be a bit unwieldy I think.

In reply to: 217836517 [](ancestors = 217836517,217834859)

Zruty0 · 2018-09-14T20:35:07Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

+            var type = inputSchema.GetColumnType(srcCol);
+            string reason = TestType(type);
+            if (reason != null)
+                //IVAN: not sure about schema mismatch


t sure about schema mismatch [](start = 26, length = 28)

no, it looks right #Resolved

Look on TestType and what kind of string it returns.
No way I can convert it to current SchemaMismatch wording.

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

oh yuck. Ok, then do ExceptParam

In reply to: 217836776 [](ancestors = 217836776,217835064)

Zruty0 · 2018-09-14T20:41:03Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

+                    var colMetaInfo = new ColumnMetadataInfo(_parent.ColumnPairs[i].output);
+                    foreach (var type in InputSchema.GetMetadataTypes(colIndex).Where(x => x.Key == MetadataUtils.Kinds.SlotNames || x.Key == MetadataUtils.Kinds.IsNormalized))
+                        Utils.MarshalInvoke(AddMetaGetter<int>, type.Value.RawType, colMetaInfo, InputSchema, type.Key, type.Value, colIndex);
+                    result[i] = new RowMapperColumnInfo(_parent.ColumnPairs[i].output, _parent._types[i], colMetaInfo);


colMetaInfo [](start = 106, length = 11)

can we just do RowColumnUtils.GetMetadataAsRow(InputSchema, colIndex, x=> x == SlotNames || x == IsNormalized) ? #Resolved

This is the whole purpose of this 'metadata as IRow' exercise, to avoid creating such 'identity getters' for metadata

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

Zruty0

Ivanidzo4ka · 2018-09-14T22:19:10Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

@@ -18,31 +18,32 @@
 using Microsoft.ML.Runtime.Internal.Utilities;
 using Microsoft.ML.Runtime.Model;
 using Microsoft.ML.Runtime.Model.Onnx;
+using Microsoft.ML.Core.Data;


using Microsoft.ML.Core.Data [](start = 0, length = 28)

Why VS can't sort it properly, why, oh why? #Resolved

after re-activating resharper, the sorting problem went away.

also, are we doing system, than Microsoft.ML or vice versa in ML.NET?

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

Let's do what Ctrl-R-G does (which is alphabetically)

In reply to: 217875183 [](ancestors = 217875183,217855717)

Ivanidzo4ka · 2018-09-14T22:22:56Z

src/Microsoft.ML.Transforms/NAHandleTransform.cs

@@ -20,28 +20,28 @@ namespace Microsoft.ML.Runtime.Data
    /// <include file='doc.xml' path='doc/members/member[@name="NAHandle"]'/>
    public static class NAHandleTransform
    {
-        public enum ReplacementKind
+        public enum ReplacementKind:byte


:byte [](start = 35, length = 5)

ctrl+k+d #Resolved

sfilipi · 2018-09-15T04:15:59Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs

-    // creating output columns that are identical to the input columns except for replacing NA values
-    // with either the default value, user input, or imputed values (min/max/mean are currently supported).
-    // Imputation modes are supported for vectors both by slot and across all slots.
-    // REVIEW: May make sense to implement the transform template interface.


why remove it? why remove the doc link? #Closed

Zruty0 · 2018-09-17T22:30:05Z

src/Microsoft.ML.Legacy/CSharpApi.cs

@@ -14444,7 +14444,7 @@ public MissingValuesRowDropperPipelineStep(Output output)

    namespace Legacy.Transforms
    {
-        public enum NAReplaceTransformReplacementKind
+        public enum NAReplaceTransformReplacementKind : byte


: byte [](start = 53, length = 7)

fix codegen? Or is it already good? #Resolved

it's already good

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

Zruty0 · 2018-09-17T22:30:41Z

test/Microsoft.ML.Tests/Transformers/NAReplaceTests.cs

+
+            var est = data.MakeNewEstimator().
+                   Append(row => (
+                   A: row.ScalarString.NAReplace(),


NAReplace [](start = 39, length = 9)

ReplaceMissingValues( #Resolved

Ivanidzo4ka · 2018-09-18T01:34:06Z

src/Microsoft.ML.Transforms/NAReplaceTransform.cs


-[assembly: LoadableClass(typeof(NAReplaceTransform), typeof(NAReplaceTransform.Arguments), typeof(SignatureDataTransform),
-   NAReplaceTransform.FriendlyName, NAReplaceTransform.LoadName, "NAReplace", NAReplaceTransform.ShortName, DocName = "transform/NAHandle.md")]
+[assembly: LoadableClass(NAReplaceTransform.Summary, typeof(IDataView), typeof(NAReplaceTransform), null, typeof(SignatureLoadDataTransform),


IDataView [](start = 60, length = 9)

IDataTransform #Resolved

sfilipi

need metadata tests //Ivan resolve and pigsty

16f44ad

Ivanidzo4ka self-assigned this Sep 14, 2018

Ivanidzo4ka added the API Issues pertaining the friendly API label Sep 14, 2018

Ivanidzo4ka added this to the 0918 milestone Sep 14, 2018

Zruty0 mentioned this pull request Sep 14, 2018

New API for ML.NET #754

Closed

some code rearrangemnts

2f79f97

Zruty0 reviewed Sep 14, 2018

View reviewed changes

Zruty0 approved these changes Sep 14, 2018

View reviewed changes

Ivan Matantsev added 2 commits September 14, 2018 14:54

some polishing

57c0774

clean code more

7ee5332

Ivanidzo4ka commented Sep 14, 2018

View reviewed changes

csharp api strike back!

0d064c3

sfilipi reviewed Sep 15, 2018

View reviewed changes

Ivan Matantsev added 2 commits September 17, 2018 14:30

Merge branch 'master' into ivanidze/NaReplace

cde365a

merge with master + pigsty

f2869fe

Zruty0 reviewed Sep 17, 2018

View reviewed changes

update

8a94270

Ivanidzo4ka commented Sep 18, 2018

View reviewed changes

Ivanidzo4ka changed the title ~~WIP NAReplace estimator~~ NAReplace estimator Sep 18, 2018

merge master

4ffac8b

Ivanidzo4ka closed this Sep 18, 2018

Ivanidzo4ka reopened this Sep 18, 2018

sfilipi approved these changes Sep 18, 2018

View reviewed changes

Ivan Matantsev added 2 commits September 18, 2018 17:27

Merge branch 'master' into ivanidze/NaReplace

f19a602

merge with master

7839b60

Ivanidzo4ka merged commit 731381c into dotnet:master Sep 19, 2018

sfilipi mentioned this pull request Oct 10, 2018

API reference - Samples for Transforms #1209

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAReplace estimator #917

NAReplace estimator #917

Ivanidzo4ka commented Sep 14, 2018

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 14, 2018

Zruty0 Sep 17, 2018

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 14, 2018

Zruty0 Sep 14, 2018

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 14, 2018

Zruty0 Sep 14, 2018

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 14, 2018

Zruty0 left a comment

Ivanidzo4ka Sep 14, 2018 •

edited

Loading

sfilipi Sep 15, 2018 •

edited

Loading

Zruty0 Sep 17, 2018

Ivanidzo4ka Sep 14, 2018 •

edited

Loading

sfilipi Sep 15, 2018 •

edited

Loading

Zruty0 Sep 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 18, 2018

Zruty0 Sep 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 18, 2018 •

edited

Loading

sfilipi left a comment

NAReplace estimator #917

NAReplace estimator #917

Conversation

Ivanidzo4ka commented Sep 14, 2018

Zruty0 Sep 14, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 14, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 14, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 14, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 14, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 14, 2018 •

edited

Loading

sfilipi Sep 15, 2018 •

edited

Loading

Ivanidzo4ka Sep 14, 2018 •

edited

Loading

sfilipi Sep 15, 2018 •

edited

Loading

Zruty0 Sep 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 18, 2018 •

edited

Loading