Replace DvText with .NET Standard type. #705

codemzs · 2018-08-21T23:01:35Z

fixes #673

…to dvtext # Conflicts: # test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj # test/Microsoft.ML.Tests/Scenarios/Api/Visibility.cs

codemzs · 2018-08-21T23:53:03Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderParser.cs

-                            scan.Span = DvText.NA;
-                        else if (_sb.Length == 0)
-                            scan.Span = DvText.Empty;
+                        if (scan.QuotingError || _sb.Length == 0)


scan.QuotingError [](start = 28, length = 17)

I wonder if throwing an exception is more appropriate here

codemzs · 2018-08-22T00:02:08Z

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

-                labelNames = new VBuffer<DvText>(2, new[] { new DvText("positive"), new DvText("negative") });
-            DvText[] names = new DvText[2];
+                labelNames = new VBuffer<ReadOnlyMemory<char>>(2, new[] { "positive".AsMemory(), "negative".AsMemory() });
+            ReadOnlyMemory<char>[] names = new ReadOnlyMemory<char>[2];


ReadOnlyMemory[] names = new ReadOnlyMemory[2]; [](start = 12, length = 59)

indent #Resolved

TomFinley · 2018-08-22T00:07:40Z

Hi @codemzs! Do note that our usual practice for "personal" code reviews is that they happen in a personal fork. That is, there is nothing preventing one from requesting a "pull request" into master in ones own fork, where one can review the changes in convenience as much as they like. Indeed this is our usual practice. #Resolved

codemzs · 2018-08-22T00:15:04Z

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

-                    var stratVals = foldCol >= 0 ? new[] { DvText.NA, DvText.NA } : new[] { DvText.NA };
+                    //REVIEW: Not sure if empty string makes sense here.
+
+                    var stratVals = foldCol >= 0 ? new[] { "".AsMemory(),"".AsMemory() } : new[] { "".AsMemory() };


Need to check on this.

…to dvtext

TomFinley · 2018-08-24T20:17:44Z

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

+        /// </summary>
+        public static string GetRawUnderlyingBufferInfo(out int ichMin, out int ichLim, ReadOnlyMemory<char> memory)
+        {
+            MemoryMarshal.TryGetString(memory, out string outerBuffer, out ichMin, out int length);


My attitude towards this is either we support ReadOnlyMemory<char> or we don't. If we do, then our methods on top of it should work, even if this fails. Fortunately it seems like this was only introduced because we wanted to just have a light "shim" on top of existing DvText methods, which is not really something we want to do. #Resolved

TomFinley · 2018-08-24T20:18:24Z

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

+        /// </summary>
+        public static bool Equals(ReadOnlyMemory<char> b, ReadOnlyMemory<char> memory)
+        {
+            if (memory.Length != b.Length)


This method and the one below could probably efficiently enough be done over direct indices over the ReadOnlyMemory<char> structure, at least, so I would hope. This would simplify the method considerably. #Resolved

TomFinley · 2018-08-24T20:19:35Z

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

+        }
+
+        /// <summary>
+        /// Does not propagate NA values. Returns true if both are NA (same as a.Equals(b)).


"Returns true if both are NA" suggests this is copy-pasted from somewhere, since of course now we will no longer have the notion of an NA string. This suggests an incomplete conversion, and is an opportunity for code simplification that we should do right now. #Resolved

TomFinley · 2018-08-24T20:36:12Z

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

+        /// Returns a text span with leading and trailing spaces trimmed. Note that this
+        /// will remove only spaces, not any form of whitespace.
+        /// </summary>
+        public static ReadOnlyMemory<char> Trim(ReadOnlyMemory<char> memory)


The two major motivations for moving from DvText to ROM<char> is that we get to avoid declaring our own special type for text, and also get to exploit the functionality that the .NET framework gives us. It seems like we did the first, and not the second: we have implementations of some things that appear to have relatively straightforward close implementations in System.MemoryExtensions. #Resolved

These extension methods are part of ReadOnlySpan and they return ROS as well which you cannot assign to ROM or create a ROM out of it and that effectively makes them useless for someone that is working with ROM. I'm starting a thread with the guys that wrote these methods to make sure I'm not missing anything.

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

TomFinley · 2018-08-24T20:37:08Z

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

+        /// <summary>
+        /// This produces zero for an empty string.
+        /// </summary>
+        public static bool TryParse(out Single value, ReadOnlyMemory<char> memory)


So, now that .NET has it, is float.TryParse(ReadOnlyMemory<char>, ...) unusable? #Resolved

If it is, you can also get rid of DoubleParser.

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

Very well. Unfortunately, these methods are only available in .netstandard 2.1, not in 2.0 and our product code targets 2.0. We cannot target 2.1 because then product code won't run on desktop framework anymore because these methods are not available there.

reference: https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.append?view=netcore-2.1

https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.append?view=netframework-4.7.2

It would have been really nice if they were though...

In reply to: 212759741 [](ancestors = 212759741,212748642)

TomFinley · 2018-08-24T21:30:06Z

    private bool TryParseCore(string text, int ich, int lim, out ulong dst)

Certainly ulong.TryParse(ReadOnlyMemory<char>, ...) exists now. So can we get rid of this? #Resolved

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:1265 in ef32b4a. [](commit_id = ef32b4a, deletion_comment = False)

TomFinley · 2018-08-24T21:30:51Z

    private bool TryParseCore(string text, int ich, int lim, out ulong dst)

Same for all TryParse methods.

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

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:1265 in ef32b4a. [](commit_id = ef32b4a, deletion_comment = False)

TomFinley · 2018-08-24T21:40:52Z

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

+        public static NormStr FindInPool(NormStr.Pool pool, ReadOnlyMemory<char> memory)
+        {
+            Contracts.CheckValue(pool, nameof(pool));
+            MemoryMarshal.TryGetString(memory, out string outerBuffer, out int ichMin, out int length);


Same comment as before. You're either supporting ReadOnlyMemor<char> or you're not. Also of course as with every other usage of this function we are ignoring whether this succeeded or not. #Resolved

TomFinley · 2018-08-24T21:51:41Z

    private bool TryParseCore(string text, int ich, int lim, out ulong dst)

You will of course have to handle mapping the empty string to 0, which things like double.TryParse and the like of course will not do, but that still represents a sizable improvement.

In reply to: 415887757 [](ancestors = 415887757,415887609)

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:1265 in ef32b4a. [](commit_id = ef32b4a, deletion_comment = False)

codemzs · 2018-09-04T22:38:05Z

    private bool TryParseCore(string text, int ich, int lim, out ulong dst)

Please see my previous reply on the other comment similar to this.

In reply to: 415892086 [](ancestors = 415892086,415887757,415887609)

Refers to: src/Microsoft.ML.Data/Data/Conversion.cs:1265 in ef32b4a. [](commit_id = ef32b4a, deletion_comment = False)

…to dvtext # Conflicts: # src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs # src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs # src/Microsoft.ML.Data/Transforms/TermTransform.cs # src/Microsoft.ML.Data/Transforms/TermTransformImpl.cs # src/Microsoft.ML.ImageAnalytics/ImageLoaderTransform.cs # test/Microsoft.ML.TestFramework/DataPipe/TestDataPipeBase.cs

…to dvtext

eerhardt · 2018-09-07T14:02:59Z

test/Microsoft.ML.Tests/TextLoaderTests.cs

@@ -237,7 +237,7 @@ public class QuoteInput
            public float ID;

            [Column("1")]
-            public string Text;
+            public ReadOnlyMemory<char> Text;


We still support regular string types, right? I think that would be bad if we didn't. #Resolved

eerhardt · 2018-09-07T14:03:47Z

test/Microsoft.ML.Tests/TextLoaderTests.cs


            [Column("1")]
-            public float Number_1;
+            public ReadOnlyMemory<char> Number_1;


This seems wrong - it was float before, and the property is called Number_1. #Resolved

eerhardt · 2018-09-07T14:05:12Z

test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj

@@ -33,6 +33,7 @@
  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="System.Memory" Version="4.5.1" />


This shouldn't be necessary. All our test projects are netcoreapp2.1, and so Span and ReadOnlyMemory should be available by default. #Resolved

eerhardt · 2018-09-07T14:05:24Z

test/Microsoft.ML.TestFramework/Microsoft.ML.TestFramework.csproj

@@ -18,4 +18,8 @@
    <NativeAssemblyReference Include="LdaNative" />
    <NativeAssemblyReference Include="FastTreeNative" />
  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="System.Memory" Version="4.5.1" />


This shouldn't be necessary. All our test projects are netcoreapp2.1, and so Span and ReadOnlyMemory should be available by default. #Resolved

eerhardt · 2018-09-07T14:07:26Z

test/Microsoft.ML.Tests/CollectionDataSourceTests.cs

-            public float? fFloat;
-            public double? fDouble;
-            public bool? fBool;
-            public string fString;


I think we should still support string types, and as such, string can be null. So we should have some tests for null strings. #Resolved

What will null string map to? empty string?

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

…to dvtext # Conflicts: # src/Microsoft.ML.Data/Microsoft.ML.Data.csproj

codemzs · 2018-09-20T18:16:25Z

This change was committed via #863

codemzs added 5 commits August 20, 2018 14:23

Util functions for MemoryReadyOnly.

b0a96a6

Replace DvText with ReadOnlyMemory.

327767b

Replace DvText with ReadOnlyMemory.

8e18252

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

73fbf9d

…to dvtext # Conflicts: # test/Microsoft.ML.Tests/Microsoft.ML.Tests.csproj # test/Microsoft.ML.Tests/Scenarios/Api/Visibility.cs

merge master and delete DvText.cs

aa0edeb

codemzs commented Aug 21, 2018

View reviewed changes

codemzs commented Aug 22, 2018

View reviewed changes

codemzs added 2 commits August 22, 2018 17:26

fix tests.

c0c0963

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

ef32b4a

…to dvtext

codemzs changed the title ~~WIP DO NOT REVIEW - Replace DvText with .NET Standard type.~~ WIP Replace DvText with .NET Standard type. Aug 23, 2018

codemzs changed the title ~~WIP Replace DvText with .NET Standard type.~~ RIP Replace DvText with .NET Standard type. Aug 23, 2018

TomFinley reviewed Aug 24, 2018

View reviewed changes

codemzs changed the title ~~RIP Replace DvText with .NET Standard type.~~ Replace DvText with .NET Standard type. Aug 26, 2018

eerhardt mentioned this pull request Aug 27, 2018

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

Merged

codemzs added 5 commits September 5, 2018 14:43

merge.

13b02f1

misc.

712ce5a

SB

1e454a6

pr feedback.

0e1dd2e

codemzs added 4 commits September 5, 2018 22:25

PR feedback.

f62b499

PR feedback.

6628d30

PR feedback.

1c698fd

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

5023f76

…to dvtext

codemzs requested a review from eerhardt September 6, 2018 19:57

codemzs added 3 commits September 6, 2018 16:19

misc.

58cdfee

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

2e5760a

…to dvtext

misc.

3bb874d

eerhardt reviewed Sep 7, 2018

View reviewed changes

codemzs added 4 commits September 7, 2018 10:45

PR feedback.

af0057e

PR feedback.

c062996

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

c578cdc

…to dvtext # Conflicts: # src/Microsoft.ML.Data/Microsoft.ML.Data.csproj

PR feedback.

8531e70

codemzs closed this Sep 20, 2018

codemzs deleted the dvtext branch September 20, 2018 18:15

TomFinley mentioned this pull request Oct 22, 2018

Make grid search of parameter space more efficient #512

Open

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 DvText with .NET Standard type. #705

Replace DvText with .NET Standard type. #705

codemzs commented Aug 21, 2018 •

edited

Loading

codemzs Aug 21, 2018

codemzs Aug 22, 2018 •

edited

Loading

TomFinley commented Aug 22, 2018 •

edited by codemzs

Loading

codemzs Aug 22, 2018

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

codemzs Sep 6, 2018

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018

codemzs Sep 4, 2018 •

edited

Loading

TomFinley commented Aug 24, 2018 •

edited by codemzs

Loading

TomFinley commented Aug 24, 2018

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley commented Aug 24, 2018

codemzs commented Sep 4, 2018

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

codemzs Sep 7, 2018

codemzs commented Sep 20, 2018

Replace DvText with .NET Standard type. #705

Replace DvText with .NET Standard type. #705

Conversation

codemzs commented Aug 21, 2018 • edited Loading

codemzs Aug 21, 2018

Choose a reason for hiding this comment

codemzs Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Aug 22, 2018 • edited by codemzs Loading

codemzs Aug 22, 2018

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Sep 6, 2018

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018

Choose a reason for hiding this comment

codemzs Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Aug 24, 2018 • edited by codemzs Loading

TomFinley commented Aug 24, 2018

TomFinley Aug 24, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

TomFinley commented Aug 24, 2018

codemzs commented Sep 4, 2018

eerhardt Sep 7, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

eerhardt Sep 7, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

eerhardt Sep 7, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

eerhardt Sep 7, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

eerhardt Sep 7, 2018 • edited by codemzs Loading

Choose a reason for hiding this comment

codemzs Sep 7, 2018

Choose a reason for hiding this comment

codemzs commented Sep 20, 2018

codemzs commented Aug 21, 2018 •

edited

Loading

codemzs Aug 22, 2018 •

edited

Loading

TomFinley commented Aug 22, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

codemzs Sep 4, 2018 •

edited

Loading

TomFinley commented Aug 24, 2018 •

edited by codemzs

Loading

TomFinley Aug 24, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading

eerhardt Sep 7, 2018 •

edited by codemzs

Loading