Replace DvBool with .NET standard type. #692

codemzs · 2018-08-18T23:24:26Z

fixes #673

Ivanidzo4ka · 2018-08-19T17:12:49Z

src/Microsoft.ML.Api/DataViewConstructionUtils.cs

-                        {
-                            Ch.Assert(colType.ItemType.IsBool);
-                            return CreateConvertingArrayGetterDelegate<bool?, DvBool>(index, x => x ?? DvBool.NA);
+                            return CreateConvertingArrayGetterDelegate<bool, bool>(index, x => x);


bool [](start = 77, length = 4)

you don't have to do it.
just fall back to default T[] -> VBuffer code. #Resolved

of course.

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

Ivanidzo4ka · 2018-08-19T17:13:23Z

src/Microsoft.ML.Api/DataViewConstructionUtils.cs

                            Ch.Assert(colType.IsBool);
-                            return CreateConvertingGetterDelegate<bool?, DvBool>(index, x => x ?? DvBool.NA);
+                            return CreateConvertingGetterDelegate<bool, bool>(index, x => x);


return CreateConvertingGetterDelegate<bool, bool>(index, x => x); [](start = 27, length = 66)

not needed, fall back to T-> T default #Resolved

yep.

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

Ivanidzo4ka · 2018-08-19T17:14:01Z

src/Microsoft.ML.Api/TypedCursor.cs

-                    {
-                        Ch.Assert(colType.ItemType.IsBool);
-                        return CreateConvertingVBufferSetter<DvBool, bool?>(input, index, poke, peek, x => (bool?)x);
+                        return CreateConvertingVBufferSetter<bool, bool>(input, index, poke, peek, x => x);


fall back to // VBuffer -> T[] #Resolved

that is correct.

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

Ivanidzo4ka · 2018-08-19T17:14:25Z

src/Microsoft.ML.Api/TypedCursor.cs

-                        Ch.Assert(colType.IsBool);
-                        Ch.Assert(peek == null);
-                        return CreateConvertingActionSetter<DvBool, bool?>(input, index, poke, x => (bool?)x);
+                        return CreateConvertingActionSetter<bool, bool>(input, index, poke, x => x);


CreateConvertingActionSetter [](start = 31, length = 28)

fall back to // T -> T #Resolved

falling back ...

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

Ivanidzo4ka · 2018-08-19T17:16:07Z

src/Microsoft.ML.Data/Data/Conversion.cs

@@ -11,10 +11,11 @@
 using System.Text;
 using System.Threading;
 using Microsoft.ML.Runtime.Internal.Utilities;
+using System.Runtime.CompilerServices;


sorting #Resolved

Ivanidzo4ka · 2018-08-19T17:16:50Z

src/Microsoft.ML.Data/Data/Conversion.cs

                dst.Append("0");
-            else if (src.IsTrue)
+            else if (src)


if (src) [](start = 17, length = 8)

this if looks redundant #Resolved

Ivanidzo4ka · 2018-08-19T17:18:26Z

src/Microsoft.ML.Data/Data/Conversion.cs

@@ -1826,8 +1821,8 @@ public void Convert(ref TX span, ref DZ value)
        public void Convert(ref BL src, ref I2 dst) => dst = (I2)src;
        public void Convert(ref BL src, ref I4 dst) => dst = (I4)src;
        public void Convert(ref BL src, ref I8 dst) => dst = (I8)src;
-        public void Convert(ref BL src, ref R4 dst) => dst = (R4)src;
-        public void Convert(ref BL src, ref R8 dst) => dst = (R8)src;
+        public void Convert(ref BL src, ref R4 dst) => dst = System.Convert.ToSingle(src);


System [](start = 61, length = 6)

you have using System; in beginning of file, is this System necessary? #Resolved

yep, it doesn't seem to recognize otherwise.

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

Ivanidzo4ka · 2018-08-19T17:23:29Z

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

@@ -596,7 +596,7 @@ public override void AttachMetadata(MetadataDispatcher.Builder bldr, ColumnType
                    Host.Check(typeSrc.RawType == typeof(TFloat));
                    bldr.AddPrimitive("CdfMean", typeSrc, Mean);
                    bldr.AddPrimitive("CdfStdDev", typeSrc, Stddev);
-                    bldr.AddPrimitive("CdfUseLog", BoolType.Instance, (DvBool)UseLog);
+                    bldr.AddPrimitive("CdfUseLog", BoolType.Instance, (bool)UseLog);


(bool) [](start = 70, length = 6)

not necessary, UseLog already bool #Resolved

Ivanidzo4ka · 2018-08-19T17:23:36Z

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

@@ -625,7 +625,7 @@ public override void AttachMetadata(MetadataDispatcher.Builder bldr, ColumnType
                    Host.Check(typeSrc.ItemType.RawType == typeof(TFloat));
                    bldr.AddGetter<VBuffer<TFloat>>("CdfMean", typeSrc, MeanMetadataGetter);
                    bldr.AddGetter<VBuffer<TFloat>>("CdfStdDev", typeSrc, StddevMetadataGetter);
-                    bldr.AddPrimitive("CdfUseLog", BoolType.Instance, (DvBool)UseLog);
+                    bldr.AddPrimitive("CdfUseLog", BoolType.Instance, (bool)UseLog);


bool [](start = 71, length = 4)

not necessary, UseLog already bool #Resolved

Ivanidzo4ka · 2018-08-19T17:27:15Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

@@ -368,8 +368,8 @@ private void FillValues(int srcLength, ref VBuffer<DvBool> dst, List<int> indice

                int ii = 0;
                // Assigns values correctly depending on the sense.
-                DvBool hit = sense ? DvBool.True : DvBool.False;
-                DvBool miss = sense ? DvBool.False : DvBool.True;
+                bool hit = sense;


bool hit = sense; [](start = 16, length = 17)

is it necessary? can you just use sense instead of hit? (I'm totally fine with miss) #Resolved

Ivanidzo4ka · 2018-08-19T17:29:02Z

test/Microsoft.ML.Core.Tests/UnitTests/DvTypes.cs

@@ -10,60 +10,6 @@ namespace Microsoft.ML.Runtime.RunTests
 {
    public sealed class DvTypeTests
    {
-        [Fact]
-        public void TestComparableDvInt4()


TestComparableDvInt4 [](start = 20, length = 20)

why you removing whole test? Is DvInt4 gone already or in this PR? #Resolved

There is another PR that removes DvInt* #Resolved

Its gone in DvInt4 PR.

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

Ivanidzo4ka · 2018-08-19T17:40:49Z

test/Microsoft.ML.Tests/CollectionDataSourceTests.cs

@@ -309,7 +309,6 @@ public class ConversionNullalbeClass
            public ulong? fuLong;
            public float? fFloat;
            public double? fDouble;
-            public bool? fBool;


public bool? fBool; [](start = 12, length = 19)

This kinda make me sad.
I understand what we want to have pure framework, and as workaround we force people to cast everything to float which support NaNs, it just in my life I worked with Something2SQL frameworks, and quite often you have nullable boolean columns, and if they fetch through DTO object, it will make that object uncastable /unwrappable for our collection to dataview.
Which will lead to bunch of painful casting/objects recreations.
But again, what do I know about life. #Resolved

yep, read Tom's comment on the issue page you might become happy! :)

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

Ivanidzo4ka · 2018-08-20T16:56:38Z

Can you link issue which trigger this PR? #Resolved

danmoseley · 2018-08-20T18:34:14Z

Are you removing DvBool.cs in a followup PR? #Resolved

eerhardt · 2018-08-20T23:10:01Z

test/Microsoft.ML.Tests/LearningPipelineTests.cs

-        }
-
-        [Fact]
-        public void NullableBooleanLabelPipeline()


I thought we decided we wanted to support bool?? #Resolved

Do we have a compelling reason?

CC: @TomFinley #Resolved

We appear to have tests that use it - https://github.com/dotnet/machinelearning/pull/692/files#diff-a18fb7e769859c74ab2d303040626b57. #Resolved

Nope, Tom, didn't think there is a compelling reason to support bool? But if you can think of something then let me know. There were two places where bool? was being used in the codebase and they were both in prediction label from score, i.e if score was null then assign predicted label as NA but we defaulted to false for missing and you can see the change in test results. Tom is ok with this.

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

yep, this is from nullable stuff that comes in from sql tables, we don;t need to support this, please see Tom;s comment on the issue comment section.

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

…to dvbool

codemzs · 2018-08-26T23:42:51Z

Yes.

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

…to dvbool

into dvbool

eerhardt · 2018-08-28T17:25:30Z

src/Microsoft.ML.Data/DataLoadSave/Binary/CodecFactory.cs

@@ -63,8 +63,7 @@ public CodecFactory(IHostEnvironment env, MemoryStreamPool memPool = null)

            // Register the old boolean reading codec.
            var oldBool = new OldBoolCodec(this);
-            RegisterOtherCodec(oldBool.LoadName, oldBool.GetCodec);
-
+            RegisterOtherCodec("DvBool", oldBool.GetCodec);


I thought we removed DvBool. ? #Resolved

Its for backward compatibility, incase someone saves a dataset in IDV format with the old DvTypes and loads in ML.NET without DvTypes.

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

We aren't doing that for the other Dv types.... Why are we doing it for DvBool?

In reply to: 213408068 [](ancestors = 213408068,213403080)

I'm doing for DvDate/Time as well. For DvInt we don't need to change because the key for codec has not changed. The key is used to retrieve the codec to parse when the loader reads the signature string.

DvBool is the only codec where we have also modified the codec as well to use one bit to write a boolean value as opposed to two bits. We need the old codec to read values from IDV that was written using DvBool.

In reply to: 213420305 [](ancestors = 213420305,213408068,213403080)

eerhardt

…to dvbool # Conflicts: # test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

…to dvbool

…to dvbool # Conflicts: # test/BaselineOutput/SingleDebug/SavePipe/TestParquetPrimitiveDataTypes-Data.txt # test/BaselineOutput/SingleDebug/SavePipe/TestParquetPrimitiveDataTypes-Schema.txt # test/BaselineOutput/SingleRelease/SavePipe/TestParquetPrimitiveDataTypes-Data.txt # test/BaselineOutput/SingleRelease/SavePipe/TestParquetPrimitiveDataTypes-Schema.txt # test/data/Parquet/alltypes.parquet

…to dvbool # Conflicts: # src/Microsoft.ML.Data/Transforms/NormalizeTransform.cs

codemzs · 2018-09-20T18:17:11Z

This change was committed via #863

codemzs added 3 commits August 18, 2018 16:09

replace DvBool with .NET standard bool type.

281f2f2

temp unblock test failure..

4385afe

clean up.

b0f624a

codemzs requested review from eerhardt, TomFinley, Ivanidzo4ka and Zruty0 August 19, 2018 00:58

codemzs changed the title ~~Replace DvBool with .NET standard for Boolean type.~~ Replace DvBool with .NET standard type. Aug 19, 2018

Ivanidzo4ka reviewed Aug 19, 2018

View reviewed changes

eerhardt reviewed Aug 20, 2018

View reviewed changes

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

5957ca5

…to dvbool

PR feedback.

92cd8c9

eerhardt mentioned this pull request Aug 27, 2018

TensorFlowMapper transform for scoring Tensorflow models in ML.NET #704

Merged

codemzs added 3 commits August 27, 2018 21:30

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

e2882ae

…to dvbool

changes.

bac9601

changes.

9adcf70

codemzs added 4 commits August 28, 2018 00:51

IDV test update.

1120e24

cleanup.

1b1f3e2

add test for parquet loader.

53b4e57

Merge branch 'parquettest' of https://github.com/codemzs/machinelearning

06c6b12

into dvbool

eerhardt reviewed Aug 28, 2018

View reviewed changes

eerhardt approved these changes Aug 28, 2018

View reviewed changes

fix build.

b0568ca

shauheen assigned codemzs Aug 28, 2018

shauheen added the enhancement New feature or request label Aug 28, 2018

shauheen added this to the 0818 milestone Aug 28, 2018

codemzs added 3 commits August 30, 2018 14:49

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

ee97d47

…to dvbool # Conflicts: # test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

misc.

924cb8a

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

b2e3916

…to dvbool

shauheen removed this from the 0818 milestone Aug 31, 2018

codemzs added 4 commits September 3, 2018 17:25

merge parquet test.

1b72099

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

ed050ec

…to dvbool # Conflicts: # src/Microsoft.ML.Data/Transforms/NormalizeTransform.cs

merge master.

ffc1f8b

codemzs closed this Sep 20, 2018

codemzs deleted the dvbool branch September 20, 2018 18:17

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

Replace DvBool with .NET standard type. #692

Replace DvBool with .NET standard type. #692

codemzs commented Aug 18, 2018 •

edited

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 19, 2018 •

edited

Loading

codemzs Aug 27, 2018

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 27, 2018

Ivanidzo4ka commented Aug 20, 2018 •

edited by codemzs

Loading

danmoseley commented Aug 20, 2018 •

edited by codemzs

Loading

eerhardt Aug 20, 2018 •

edited by codemzs

Loading

codemzs Aug 21, 2018 •

edited

Loading

eerhardt Aug 21, 2018 •

edited by codemzs

Loading

codemzs Aug 26, 2018

codemzs Aug 26, 2018

codemzs commented Aug 26, 2018

eerhardt Aug 28, 2018 •

edited by codemzs

Loading

codemzs Aug 28, 2018

eerhardt Aug 28, 2018

codemzs Aug 28, 2018

eerhardt left a comment

codemzs commented Sep 20, 2018

Replace DvBool with .NET standard type. #692

Replace DvBool with .NET standard type. #692

Conversation

codemzs commented Aug 18, 2018 • edited Loading

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Aug 20, 2018 • edited by codemzs Loading

danmoseley commented Aug 20, 2018 • edited by codemzs Loading

eerhardt Aug 20, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

eerhardt Aug 21, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemzs commented Aug 26, 2018

eerhardt Aug 28, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

codemzs commented Sep 20, 2018

codemzs commented Aug 18, 2018 •

edited

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

codemzs Aug 19, 2018 •

edited

Loading

Ivanidzo4ka Aug 19, 2018 •

edited by codemzs

Loading

Ivanidzo4ka commented Aug 20, 2018 •

edited by codemzs

Loading

danmoseley commented Aug 20, 2018 •

edited by codemzs

Loading

eerhardt Aug 20, 2018 •

edited by codemzs

Loading

codemzs Aug 21, 2018 •

edited

Loading

eerhardt Aug 21, 2018 •

edited by codemzs

Loading

eerhardt Aug 28, 2018 •

edited by codemzs

Loading