Replace predicate with an IEnumerable<DataViewSchema.Column> for IRowToRowMapper.GetRow and ISchemaBoundRowMapper.GetRow #2796

sfilipi · 2019-02-28T17:52:03Z

Note to reviewers: The first commit does all the replacement work.
The second commit, has more files on it, because it deals with renaming the col to columnIndex in the other two methods of IDataView (@eerhardt comment on #1529)
The third commit is me reviewing the first commit, and making sure that GetRow is an explicit implementation everywhere.

I have had most of the work on this PR done before breaking ISchemaBoundRowMapper deriving from IRowToRowMapper; so I kept the signature change of ISchemaBoundRowMapper.GetRow although that interface is internal. LMK if that is not desirable.

Ivanidzo4ka · 2019-03-01T05:25:52Z

src/Microsoft.Data.DataView/IDataView.cs


        /// <summary>
-        /// Returns a value getter delegate to fetch the given column value from the row.
+        /// Returns a value getter delegate to fetch the valueof column with the given columnIndex, from the row.


valueof [](start = 57, length = 7)

space #Resolved

Ivanidzo4ka · 2019-03-01T05:26:31Z

src/Microsoft.Data.DataView/IDataView.cs


        /// <summary>
-        /// Returns a value getter delegate to fetch the given column value from the row.
+        /// Returns a value getter delegate to fetch the valueof column with the given columnIndex, from the row.


columnIndex [](start = 87, length = 11)

paramref? #Resolved

Ivanidzo4ka · 2019-03-01T05:27:51Z

src/Microsoft.ML.Core/Data/AnnotationUtils.cs

-            public override ValueGetter<TValue> GetGetter<TValue>(int col) => _annotations.GetGetter<TValue>(col);
+
+            /// <summary>
+            /// Returns a value getter delegate to fetch the valueof column with the given columnIndex, from the row.


valueof [](start = 61, length = 7)

space, I guess across all files. #Resolved

Ivanidzo4ka · 2019-03-01T05:43:11Z

src/Microsoft.ML.Data/DataView/AppendRowsDataView.cs

-                Ch.Check(IsColumnActive(col), "The column must be active against the defined predicate.");
-                if (!(Getters[col] is ValueGetter<TValue>))
+                Ch.Check(IsColumnActive(columnIndex), "The column must be active against the defined predicate.");
+                if (!(Getters[columnIndex] is ValueGetter<TValue>))


Ch.CheckParam(IsColumnActive(columnIndex), nameof(columnIndex), "requested column not active");
from transposeLoader.cs
I like it more than some mythical defined predicate #Resolved

Ivanidzo4ka · 2019-03-01T05:46:06Z

src/Microsoft.ML.Data/DataView/AppendRowsDataView.cs

-                Ch.Check(0 <= col && col < Schema.Count, "Column index is out of range");
-                return Getters[col] != null;
+                Ch.Check(0 <= columnIndex && columnIndex < Schema.Count, "Column index is out of range");
+                return Getters[columnIndex] != null;


Ch.CheckParam(0 <= columnIndex && columnIndex < _colToActive.Length, nameof(columnIndex));
in files above this one we use this format. consistency would be nice. not a blocker tho. #Pending

this does not have _colToActive. Leaving it.

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

Ivanidzo4ka · 2019-03-01T05:46:57Z

src/Microsoft.ML.Data/DataView/ArrayDataViewBuilder.cs

-                    Ch.Check(_active[col], "column is not active");
-                    var column = _view._columns[col] as Column<TValue>;
+                    Ch.Check(0 <= columnIndex & columnIndex < Schema.Count);
+                    Ch.Check(_active[columnIndex], "column is not active");


"column is not active" [](start = 51, length = 22)

for the sake of consistency: "requested column not active" #Resolved

Ivanidzo4ka · 2019-03-01T05:49:01Z

src/Microsoft.ML.Data/DataView/CacheDataView.cs

-                    throw Ch.Except("Column #{0} is requested but not active in the cursor", col);
-                var getter = _getters[_colToActivesIndex[col]] as ValueGetter<TValue>;
+                if (!IsColumnActive(columnIndex))
+                    throw Ch.Except("Column #{0} is requested but not active in the cursor", columnIndex);


in how many different ways we can do same thing :)
Ch.CheckParam(IsColumnActive(columnIndex), nameof(columnIndex), "requested column not active");? #Resolved

in many.

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

Ivanidzo4ka · 2019-03-01T05:59:40Z

src/Microsoft.ML.Data/DataView/CompositeRowToRowMapper.cs

@@ -49,18 +49,19 @@ IEnumerable<DataViewSchema.Column> IRowToRowMapper.GetDependencies(IEnumerable<D
            return columnsNeeded;
        }

-        public DataViewRow GetRow(DataViewRow input, Func<int, bool> active)
+        DataViewRow IRowToRowMapper.GetRow(DataViewRow input, IEnumerable<DataViewSchema.Column> activeColumns)


IEnumerable<DataViewSchema.Column> activeColumns [](start = 62, length = 48)

It's probably quite late considering how much work you did already for this issue.
So we replace functions to schema columns.
In past we had just indexes and we just check are they belong to schema range or not.
Now we accept schema columns. But we use only index position in them.
Which can lead to situation where i have two dataviews with following schemas:
one is A: int, B: float, and another is C:string, D: bool.
I want to get rows from first dataview. but I made mistake and I pass schema from second dataview as active columns.
I would assume it would work perfectly fine in current state of the code. I'm not sure it should.
#Pending

Agree. should switch from tracking activeIndices to active columns at some point. Logged: #2816

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

Ivanidzo4ka · 2019-03-01T06:20:11Z

src/Microsoft.ML.Data/Prediction/Calibrator.cs

                }
-                if (predicate(OutputSchema.Count - 1))
+
+                if (activeColumns.Select(c => c.Name).Contains(DefaultColumnNames.Probability))


if (activeColumns.Select(c => c.Name).Contains(DefaultColumnNames.Probability)) [](start = 15, length = 80)

I would rather change if inside foreach loop to set bool flag to true if column.Index == OutputSchema.Count-1 instead of continue and check that flag here.
You have mix of two different conditions right now.
And who knows maybe we will allow user to define their own names for probability column in future, so index feels somewhat more secure. #Resolved

Ivanidzo4ka · 2019-03-01T06:22:04Z

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

@@ -385,9 +385,7 @@ public DataViewRow GetRow(DataViewRow input, Func<int, bool> active)
            }

            public Func<int, bool> GetGenericPredicate(Func<int, bool> predicate)


GetGenericPredicate [](start = 35, length = 19)

is it still needed? #Resolved

Ivanidzo4ka · 2019-03-01T06:23:35Z

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

                var totalColumnsCount = 1 + _outputGenericSchema.Count;
                var getters = new Delegate[totalColumnsCount];

-                if (active(totalColumnsCount - 1))
+                if (activeColumns.Select(c => c.Index).Contains(totalColumnsCount - 1))


totalColumnsCount - 1 [](start = 64, length = 21)

this is _outputGenericSchema.Count :) #Resolved

Ivanidzo4ka · 2019-03-01T06:28:50Z

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

        {
-            Ch.Check(0 <= col & col < _bindings.ColumnCount, "col");
+            Ch.Check(0 <= columnIndex & columnIndex < _bindings.ColumnCount, "col");


"col" [](start = 77, length = 5)

nameof! #Resolved

Ivanidzo4ka · 2019-03-01T07:03:35Z

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

        {
            Func<int, bool> activeInfos =
                iinfo =>
                {
                    int col = _bindings.MapIinfoToCol(iinfo);
-                    return active(col);
+                    return activeColumns.Select(c => c.Index).Contains(col);


activeColumns.Select(c => c.Index) [](start = 27, length = 34)

Make a hashset before you define activeInfos and use hashSet.Contains here.
#Resolved

thanks.

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

Ivanidzo4ka · 2019-03-01T07:10:31Z

src/Microsoft.ML.TimeSeries/PredictionFunction.cs


            IRowToRowMapper[] innerMappers = new IRowToRowMapper[0];
            if (mapper is CompositeRowToRowMapper compositeMapper)
                innerMappers = compositeMapper.InnerMappers;

+            var activeIndices = activeColumns.Select(c => c.Index);


ToArray or HashSet. or Utils.BuildArray #Resolved

Ivanidzo4ka · 2019-03-01T07:13:56Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

        {
            Func<int, bool> activeInfos =
                iinfo =>
                {
                    int col = _bindings.MapIinfoToCol(iinfo);
-                    return active(col);
+                    return activeColumns.Select(c => c.Index).Contains(col);


activeColumns.Select(c => c.Index) [](start = 27, length = 34)

hashset #Resolved

Ivanidzo4ka · 2019-03-01T07:22:31Z

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

-            Func<int, bool> activeInfos = iinfo => active(bindings.MapIinfoToCol(iinfo));
+            GetActive(bindings, activeColumns, out inputColumns, out IEnumerable<DataViewSchema.Column> activeMapperColumns);
+            var output = bindings.RowMapper.GetRow(input, activeMapperColumns);
+            Func<int, bool> activeInfos = iinfo => activeColumns.Select(c => c.Index).Contains(bindings.MapIinfoToCol(iinfo));


activeColumns [](start = 51, length = 13)

this can be quite heavy #Pending

eerhardt · 2019-03-01T13:28:23Z

src/Microsoft.Data.DataView/IDataView.cs

        /// </summary>
-        public abstract bool IsColumnActive(int col);
+        public abstract bool IsColumnActive(int columnIndex);


I thought we were changing these two methods to take in the column instead of the index?

public abstract bool IsColumnActive(DataViewSchema.Column column); #Resolved

I think the predicates were deemed hard to debug. Ideally we'd pass the column and not deal with the indexes but is that a V1 issue? @shauheen @glebuk @TomFinley #Resolved

The issue being closed here says:

The use of integer indices here has sometimes led to confusion or even bugs.

Which seems to me that we shouldn't have integer indices in our public API. So if we ship V1 like this, we won't be able to remove/change this API. We can add a new virtual method that takes in the Column, but this current API will need to live forever. #Resolved

sfilipi · 2019-03-05T18:40:59Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

@@ -1153,7 +1155,7 @@ private static IDataView AddVarLengthColumn<TSrc>(IHostEnvironment env, IDataVie
                            throw Contracts.Except("Multiple unweighted rows found in metrics data view.");

                        numResults++;
-                        UpdateSums(isWeightedCol, stratCol, stratVal, agg, numFolds > 1, metricNames, hasWeighted, hasStrats,
+                        UpdateSums(isWeightedColumn.Value.Index, stratCol, stratVal, agg, numFolds > 1, metricNames, hasWeighted, hasStrats,


switch to isWeightedCol #Resolved

sfilipi · 2019-03-05T18:46:56Z

src/Microsoft.Data.DataView/IDataView.cs

@@ -132,16 +132,18 @@ public abstract class DataViewRow : IDisposable
        public abstract ValueGetter<DataViewRowId> GetIdGetter();

        /// <summary>
-        /// Returns whether the given column is active in this row.
+        /// Returns whether the give column is active in this row.


fix #Resolved

sfilipi · 2019-03-05T18:49:19Z

try removing this, as it has only one reference. #WontFix

Refers to: src/Microsoft.ML.Core/Utilities/Utils.cs:847 in 7aa80a1. [](commit_id = 7aa80a1, deletion_comment = False)

sfilipi · 2019-03-05T18:50:54Z

src/Microsoft.ML.Data/Data/DataViewUtils.cs

@@ -523,7 +523,7 @@ private DataViewRowCursor[] SplitCore(IChannelProvider ch, DataViewRowCursor inp
                // Create the mappings between active column index, and column index.
                int[] activeToCol;
                int[] colToActive;
-                Utils.BuildSubsetMaps(_schema.Count, input.IsColumnActive, out activeToCol, out colToActive);
+                Utils.BuildSubsetMaps(_schema.Count, _schema, input.IsColumnActive, out activeToCol, out colToActive);


_schema.Count [](start = 38, length = 13)

try removing the count, if we're passing schema #Resolved

sfilipi · 2019-03-05T19:08:36Z

it is too involved atm.

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

Refers to: src/Microsoft.ML.Core/Utilities/Utils.cs:847 in 7aa80a1. [](commit_id = 7aa80a1, deletion_comment = False)

eerhardt · 2019-03-05T19:14:06Z

src/Microsoft.Data.DataView/IDataView.cs

        /// This throws if the column is not active in this row, or if the type
        /// <typeparamref name="TValue"/> differs from this column's type.
        /// </summary>
-        public abstract ValueGetter<TValue> GetGetter<TValue>(int col);
+        /// <typeparam name="TValue"> is the output column's content type.</typeparam>


(nit) Is it confusing to say output column here and below? To the consumer of an IDataView, they are just columns. #Resolved

eerhardt · 2019-03-05T19:14:58Z

src/Microsoft.Data.DataView/SchemaDebuggerProxy.cs

@@ -50,7 +50,7 @@ public AnnotationsDebuggerProxy(DataViewSchema.Annotations annotations)
        private static object GetValue<T>(DataViewSchema.Annotations annotations, int columnIndex)
        {
            T value = default;
-            annotations.GetGetter<T>(columnIndex)(ref value);
+            annotations.GetGetter<T>(annotations.Schema[columnIndex])(ref value);


This method is only called in one place - which already has the Column. Can we just pass the column through here? #Closed

TomFinley · 2019-03-06T19:36:58Z

src/Microsoft.Data.DataView/SchemaDebuggerProxy.cs

@@ -41,16 +41,16 @@ public AnnotationsDebuggerProxy(DataViewSchema.Annotations annotations)
            foreach (var column in annotations.Schema)
            {
                var name = column.Name;
-                var value = Utils.MarshalInvoke(GetValue<int>, column.Type.RawType, annotations, column.Index);
+                var value = Utils.MarshalInvoke(GetValue<DataViewSchema.Column>, column.Type.RawType, annotations, column);


DataViewSchema.Column [](start = 57, length = 21)

Incidentally this was not necessary or helpful. The point is to merely have a method for the method to sink its teeth into, so the type parameter here was not informative. (If anything it is now more obscure.)

This is not essential -- the method will still work the same even with this change, and there should be no cost to it. Just pointing out that there was no need for it.

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

TomFinley · 2019-03-06T19:41:04Z

src/Microsoft.ML.Core/Utilities/Utils.cs

+        /// so that <c><paramref name="invMap"/>[<paramref name="map"/>[i]] == i</c>,
+        /// and for other entries not appearing in <paramref name="map"/>,
+        /// <c><paramref name="invMap"/>[i] == -1</c></param>
+        public static void BuildSubsetMaps(DataViewSchema schema, Func<DataViewSchema.Column, bool> pred, out int[] map, out int[] invMap)


Func<DataViewSchema.Column, bool> pred [](start = 66, length = 38)

Hmmm. Is the goal here to enable more elaborate checking I suppose? Totally fine, just interesting.

TomFinley · 2019-03-06T19:45:28Z

Reviewing this reminds me, we'll have to deal with ITransposeDataView sooner or later. But, maybe later.

TomFinley

Thank you @sfilipi !

eerhardt

…<DataViewSchema.Column> activeColumns` from the IRowToRowMapper.GetRow and the ISchemaBoundRowMapper.GetRow. renaming col to `columnIndex` in the `DataViewRow.IsColumnActive` and `DataViewRow.GetGetter<TValue>`

…er and DataView.IsColumnActive Documentation.

… take a column, rather than column index.

…lumn instead of the column index.

sfilipi requested review from Ivanidzo4ka, eerhardt, TomFinley and yaeldekel February 28, 2019 17:52

sfilipi self-assigned this Feb 28, 2019

sfilipi added the API Issues pertaining the friendly API label Feb 28, 2019

Ivanidzo4ka reviewed Mar 1, 2019

View reviewed changes

eerhardt reviewed Mar 1, 2019

View reviewed changes

sfilipi mentioned this pull request Mar 1, 2019

Follow up on removing the predicates from the IDataView methods signatures #2816

Open

sfilipi force-pushed the GetRowRemovePredicate branch from 0ccbde7 to 7aa80a1 Compare March 5, 2019 06:11

sfilipi commented Mar 5, 2019

View reviewed changes

eerhardt reviewed Mar 5, 2019

View reviewed changes

TomFinley reviewed Mar 6, 2019

View reviewed changes

TomFinley approved these changes Mar 6, 2019

View reviewed changes

eerhardt approved these changes Mar 7, 2019

View reviewed changes

sfilipi added 6 commits March 7, 2019 14:43

replacing the Func<int, bool> active predicate with an `IEnumerable…

8c32f37

…<DataViewSchema.Column> activeColumns` from the IRowToRowMapper.GetRow and the ISchemaBoundRowMapper.GetRow. renaming col to `columnIndex` in the `DataViewRow.IsColumnActive` and `DataViewRow.GetGetter<TValue>`

renaming col to columnIndex in all the overrides of IDataView.GetGett…

7da0875

…er and DataView.IsColumnActive Documentation.

Making sure GetRow is an explicit implementation everywhere.

784ab37

addressing PR review, and changing the GetGetter method signature, to…

f5b4664

… take a column, rather than column index.

changing the signature for IsColumnActive to take a DataViewSchema.Co…

40ce785

…lumn instead of the column index.

pr review comments

5489b19

sfilipi force-pushed the GetRowRemovePredicate branch from 230ce75 to 5489b19 Compare March 7, 2019 22:47

sfilipi merged commit 683711a into dotnet:master Mar 8, 2019

sfilipi deleted the GetRowRemovePredicate branch March 8, 2019 06:16

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

		@@ -385,9 +385,7 @@ public DataViewRow GetRow(DataViewRow input, Func<int, bool> active)
		}

		public Func<int, bool> GetGenericPredicate(Func<int, bool> predicate)

Replace predicate with an IEnumerable<DataViewSchema.Column> for IRowToRowMapper.GetRow and ISchemaBoundRowMapper.GetRow #2796

Replace predicate with an IEnumerable<DataViewSchema.Column> for IRowToRowMapper.GetRow and ISchemaBoundRowMapper.GetRow #2796

Conversation

sfilipi commented Feb 28, 2019

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 1, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 1, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 1, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

eerhardt Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Mar 1, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi commented Mar 5, 2019 • edited Loading

sfilipi Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi commented Mar 5, 2019

eerhardt Mar 5, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

eerhardt Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Mar 6, 2019

Choose a reason for hiding this comment

TomFinley Mar 6, 2019

Choose a reason for hiding this comment

TomFinley Mar 6, 2019

Choose a reason for hiding this comment

TomFinley commented Mar 6, 2019

TomFinley left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

sfilipi Mar 1, 2019 •

edited

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Mar 1, 2019 •

edited by sfilipi

Loading

eerhardt Mar 1, 2019 •

edited by sfilipi

Loading

sfilipi Mar 1, 2019 •

edited

Loading

eerhardt Mar 1, 2019 •

edited by sfilipi

Loading

sfilipi Mar 5, 2019 •

edited

Loading

sfilipi Mar 5, 2019 •

edited

Loading

sfilipi commented Mar 5, 2019 •

edited

Loading

sfilipi Mar 5, 2019 •

edited

Loading

eerhardt Mar 5, 2019 •

edited by sfilipi

Loading

eerhardt Mar 5, 2019 •

edited

Loading