Update input arguments in CpuMath files by jwood803 · Pull Request #1021 · dotnet/machinelearning

jwood803 · 2018-09-25T14:59:54Z

Fix for #824

Marked as WIP since this may have more updates to come.

@briancylui @eerhardt @tannergooding Feel free to let me know if anything already done should be changed.

tannergooding · 2018-09-25T15:11:47Z

            => Avx.IsSupported ? Vector256Alignment : (Sse.IsSupported ? Vector128Alignment : FloatAlignment);

-        public static void MatTimesSrc(bool tran, bool add, AlignedArray mat, AlignedArray src, AlignedArray dst, int crun)
+        public static void MatTimesSource(bool transpose, bool add, AlignedArray mat, AlignedArray source, AlignedArray destination, int crun)


mat here stands for matrix

Oh, thanks! I'll update that.

crun is also attempting to indicate how many columns are in each row of the matrix. We could probably have a better name here.... stride is the term I'm more generally familiar with.

eerhardt · 2018-09-25T15:24:28Z

        }

-        private static void Scale(float a, Span<float> dst)
+        private static void Scale(float source, Span<float> destination)


These names aren't really correct. The source parameter isn't really the source of the information. It is the amount to scale the dst values.

Maybe a should just be renamed to scale?

Or possibly value?

Similar comment for the Add method above.

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

I think value sounds good. I'll update that.

tannergooding · 2018-09-25T16:49:24Z


-        public static void MatTimesSrc(bool tran, bool add, AlignedArray mat, int[] rgposSrc, AlignedArray srcValues,
-            int posMin, int iposMin, int iposLim, AlignedArray dst, int crun)
+        public static void MatrixTimesSource(bool transpose, bool add, AlignedArray mat, int[] rgpossource, AlignedArray sourceValues,


We shouldn't need the rg prefix, we can just use posSource.
This mat (and most of the others in the file) also stands for matrix

tannergooding · 2018-09-25T16:51:43Z

-        public static void MatTimesSrc(bool tran, bool add, AlignedArray mat, int[] rgposSrc, AlignedArray srcValues,
-            int posMin, int iposMin, int iposLim, AlignedArray dst, int crun)
+        public static void MatrixTimesSource(bool transpose, bool add, AlignedArray mat, int[] rgpossource, AlignedArray sourceValues,
+            int posMin, int iposMin, int iposLim, AlignedArray destination, int stride)


I believe the Lim here stands for Limit

tannergooding · 2018-09-25T16:52:22Z

        }

-        private static void Scale(float a, Span<float> src, Span<float> dst)
+        private static void Scale(float a, Span<float> source, Span<float> destination)


a can be value

tannergooding · 2018-09-25T16:53:05Z

-        // dst[i] = a * (dst[i] + b)
-        public static void ScaleAdd(float a, float b, float[] dst, int count)
+        // destination[i] = a * (destination[i] + b)
+        public static void ScaleAdd(float a, float b, float[] destination, int count)


Maybe:

a => scale

b => addend
?

tannergooding · 2018-09-25T16:53:39Z

        }

-        public static void MulElementWise(float[] src1, float[] src2, float[] dst, int count)
+        public static void MulElementWise(float[] source1, float[] source2, float[] destination, int count)


left and right are probably better names here

tannergooding · 2018-09-25T16:54:19Z

This is a great improvement and helps increase readability immensely. Thanks for the work @jwood803

jwood803 · 2018-09-26T12:10:26Z

Glad to help @tannergooding! I hope to do more in the future 😄

eerhardt · 2018-09-26T14:53:29Z

@jwood803 - are there more updates coming? Or can the WIP be removed and make this an official PR?

jwood803 · 2018-09-26T14:59:55Z

@eerhardt Good question. I can go ahead and remove the WIP and if you or @tannergooding has any more feedback, I can just make changes then.

eerhardt · 2018-09-27T15:52:30Z

            => Avx.IsSupported ? Vector256Alignment : (Sse.IsSupported ? Vector128Alignment : FloatAlignment);

-        public static void MatTimesSrc(bool tran, bool add, AlignedArray mat, AlignedArray src, AlignedArray dst, int crun)
+        public static void MatrixTimesSource(bool transpose, bool add, AlignedArray matrix, AlignedArray source, AlignedArray destination, int stride)


This is a partial class, and the .netstandard.cs side of the partial class needs to be updated as well.

BTW - are you compiling for .NET Core 3.0? I assume there are callers of this function that need to be updated. I have 3.0 building instructions in this soon to be merged PR: #1032

Thanks, @eerhardt! I totally didn't realize it was a partial class. The netstandard class has been updated as well as the callers of the functions that were changed.

I'll mess with the .NET Core 3.0 part to make sure all that looks good.

@eerhardt Tried to compile for .NET Core 3.0 and, with the VS route, it seemed that everything in the CpuMath library fails to compile.

I also tried through the command line and would get this error:

I'm sure I probably did a step wrong so I'll continue to mess with it, but I figured I'd give an update.

Something is eating your / in /p:Configuration=Release-Intrinsics on the command line. What shell are you using? CMD? Powershell?

try specifying it with a - instead: -p:Configuration=Release-Intrinsics

That worked better! I use bash on Cmder for my terminal.

It looks like it builds for the most part, but I do get the error below. Even after running dotnet restore it still comes up. I wouldn't think it has anything to do with these changes, though.

C:\Users\jwood\Documents\Code\GitHub\machinelearning\Tools\dotnetcli\sdk\3.0.100-alpha1-009632\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(202,5): error NETSDK1004: Assets file 'C:\Users\jwood\Documents\Code\GitHub\machinelearning\bin\obj\AnyCPU.Release-Intrinsics\Microsoft.ML.CpuMath\project.assets.json' not found. Run a NuGet package restore to generate this file. [C:\Users\jwood\Documents\Code\GitHub\machinelearning\src\Microsoft.ML.CpuMath\Microsoft.ML.CpuMath.csproj]

Do you have my latest code: #1032? That should solve that problem.

Great catch, @eerhardt! Turns out I did miss some updates from a previous change. Thanks!

eerhardt · 2018-10-01T18:53:06Z

        }

-        [Fact(Skip = "Execute this test if you want to regenerate ep-list and _manifest.json")]
+        [Fact]


this shouldn't be checked in.

Whoops! I'll revert that.

eerhardt · 2018-10-01T18:54:19Z

+        public static float DotProductSparse(float[] value, int offset, float[] b, int[] indices, int count) => SseUtils.DotProductSparse(value, offset, b, indices, count);

-        public static float L2DistSquared(float[] a, float[] b, int count) => SseUtils.L2DistSquared(a, b, count);
+        public static float L2DistSquared(float[] value, float[] b, int count) => SseUtils.L2DistSquared(value, b, count);


b should be updated as well. Similar comment throughout this code file.

Actually, I don't think value makes sense for the first parameter either.

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

Would source and destination make sense there?

Not for the specific method, no. This is computing the distance between two arrays and returning a float. So there isn't a destination array.

left and right would make sense. Or a and b... but I think you are changing it to use left and right elsewhere, so let's be consistent.

danmoseley · 2018-10-23T23:50:29Z

@jwood803 do you have time to rebase and resolve conflicts?

danmoseley · 2018-10-23T23:52:01Z

@jwood803 depending, it might be easier to repeat the search and replace in some cases.

danmoseley · 2018-10-25T00:26:07Z

@tannergooding can you help @jwood803 this PR get closed out one way or another?

jwood803 · 2018-10-28T20:31:51Z

Apologies for the long delay everyone. I did the merge and hopefully, all will turn out ok, but I'll update any issues that may arise from it. Thanks for the patience, everyone!

eerhardt · 2018-10-29T15:12:12Z

@jwood803 - it looks like your merge wasn't successful -

2018-10-28T20:35:35.4153390Z CpuMathUtils.netstandard.cs(22,133): error CS0103: The name 'srcValues' does not exist in the current context [/__w/1/s/src/Microsoft.ML.CpuMath/Microsoft.ML.CpuMath.csproj]
2018-10-28T20:35:35.4154682Z CpuAligenedMathUtils.cs(88,26): error CS0117: 'CpuMathUtils' does not contain a definition for 'MatrixTimesSource' [/__w/1/s/src/Microsoft.ML.CpuMath/Microsoft.ML.CpuMath.csproj]
2018-10-28T20:35:35.4155112Z CpuAligenedMathUtils.cs(103,26): error CS0117: 'CpuMathUtils' does not contain a definition for 'MatrixTimesSource' [/__w/1/s/src/Microsoft.ML.CpuMath/Microsoft.ML.CpuMath.csproj]
2018-10-28T20:35:35.4155218Z /__w/1/s/dir.traversal.targets(25,5): error : Build failed. See earlier errors. [/__w/1/s/build.proj]

Can you fix the build errors?

eerhardt · 2018-10-29T15:13:39Z

            private ValueGetter<VBuffer<float>> GetterFromFloatType(IRow input, int iinfo)
            {
+
                var getSrc = input.GetGetter<float>(_srcCols[iinfo]);


Can you revert the unnecessary blank lines being added here?

eerhardt · 2018-10-29T15:14:01Z


        [DllImport(NativePath), SuppressUnmanagedCodeSecurity]
+
        public static extern void SdcaL1UpdateU(float primalUpdate, /*const*/ float* ps, float threshold, float* pd1, float* pd2, int c);


Can you revert the unnecessary blank line added?

jwood803 · 2018-10-29T15:16:13Z

@eerhardt Sorry about that. I'll get those updated.

eerhardt

shauheen · 2018-10-31T23:57:15Z

@tannergooding can you also review the PR?

danmoseley · 2018-11-02T17:34:10Z

        }

-        public static void SdcaL1UpdateSparse(float primalUpdate, int count, ReadOnlySpan<float> src, ReadOnlySpan<int> indices, float threshold, Span<float> v, Span<float> w)
+        public static void SdcaL1UpdateSparse(float primalUpdate, int count, ReadOnlySpan<float> source, ReadOnlySpan<int> indices, float threshold, Span<float> v, Span<float> w)


I don't know what v and w stand for throughout, but in some future PR someone might want to make them more descriptive...

danmoseley · 2018-11-02T17:54:33Z

+            int posMin, int iposMin, int iposLimit, AlignedArray destination, int stride) => SseUtils.MatTimesSrc(matrix, rgposSrc, sourceValues, posMin, iposMin, iposLimit, destination, stride);

-        public static void Add(float a, Span<float> dst) => SseUtils.Add(a, dst);
+        public static void Add(float value, Span<float> destination) => SseUtils.Add(value, destination);


When something is being added it seems like sometimes we call it value and sometimes addend. And if we want to make them consistently addend maybe value should be more specific in other cases, such as scale. Eg.,
Add(float addend, Span<float> destination)
ScaleAdd(float scale, float addend, Span<float> destination)
AddScale(float scale, ReadOnlySpan<float> addends, Span<float> destination, int count)

But I defer to @tannergooding as I haven't given this previous thought and I don't know what the convention is. Just as long as we're consistent.

I don't think that should stop merging this PR though.

danmoseley · 2018-11-02T18:04:12Z

Seems fine but I'll let @tannergooding sign off.

shauheen · 2018-11-06T23:13:04Z

ping @tannergooding for review.

tannergooding

LGTM

jwood803 · 2018-11-07T16:09:27Z

Much thanks to @eerhardt and @tannergooding for all the help on this PR! 😄

jwood803 added 6 commits September 17, 2018 17:31

Start renaming arguments

a28701b

Merge branch 'master' into cpu-rename-arguments

00c5cdb

Start updating parameter names

177edd9

Update parameter names

70226ba

Merge branch 'master' into cpu-rename-arguments

eb42375

Update naming of method

5926e60

tannergooding reviewed Sep 25, 2018

View reviewed changes

eerhardt reviewed Sep 25, 2018

View reviewed changes

Update argument names due to PR feedback

6ee9669

tannergooding reviewed Sep 25, 2018

View reviewed changes

Update more argument names

82542b7

jwood803 changed the title ~~WIP: Update input arguments in CpuMath files~~ Update input arguments in CpuMath files Sep 26, 2018

Remove unused argument

287f7fc

eerhardt reviewed Sep 27, 2018

View reviewed changes

jwood803 added 3 commits October 1, 2018 12:41

Update net standard file

d6a677d

Merge branch 'master' into cpu-rename-arguments

d228f73

Regen entry points after merging

aa93772

eerhardt reviewed Oct 1, 2018

View reviewed changes

jwood803 added 3 commits October 1, 2018 16:50

Update more argument names and revert test skip

0ab973b

Merge branch 'master' into cpu-rename-arguments

60fb9cd

Fix compile issues in .NET Core compile

dc99725

jwood803 added 5 commits October 5, 2018 18:44

Remove unused parameter

0a6ce77

Fix build errors for removing length parameter

ca153eb

Merge from master

9f4c267

Update test arguments

dcbaf69

Update more argument names

2a1a8dd

danmoseley assigned tannergooding Oct 23, 2018

Merge branch 'master' into cpu-rename-arguments

d01b460

eerhardt reviewed Oct 29, 2018

View reviewed changes

jwood803 added 3 commits October 29, 2018 17:48

Fix merge issues

46b31a9

Merge branch 'master' into cpu-rename-arguments

2ae946a

Further PR updates

3fe14e6

eerhardt approved these changes Oct 30, 2018

View reviewed changes

danmoseley reviewed Nov 2, 2018

View reviewed changes

Comment thread src/Microsoft.ML.CpuMath/CpuMathUtils.netcoreapp.cs

tannergooding approved these changes Nov 7, 2018

View reviewed changes

shauheen merged commit a3b09bf into dotnet:master Nov 7, 2018

jwood803 deleted the cpu-rename-arguments branch November 7, 2018 14:42

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


		[DllImport(NativePath), SuppressUnmanagedCodeSecurity]

		public static extern void SdcaL1UpdateU(float primalUpdate, /const/ float* ps, float threshold, float* pd1, float* pd2, int c);

Uh oh!

Conversation

jwood803 commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Sep 25, 2018

Uh oh!

jwood803 commented Sep 26, 2018

Uh oh!

eerhardt commented Sep 26, 2018

Uh oh!

jwood803 commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Oct 23, 2018

Uh oh!

danmoseley commented Oct 23, 2018

Uh oh!

danmoseley commented Oct 25, 2018

Uh oh!

jwood803 commented Oct 28, 2018

Uh oh!

eerhardt commented Oct 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwood803 commented Oct 29, 2018

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

shauheen commented Oct 31, 2018