Skip to content

Add doc string to explain matrix-vector product's SSE code and a test #3124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 11, 2019

Conversation

wschin
Copy link
Member

@wschin wschin commented Mar 27, 2019

CpuMath library is not documented, so I add one for its matrix-vector product. Other functions look easier. A new test based on matrix factorization is also added --- we factorize training matrix into two factor matrices and then use CpuMath to compute their product.

Also fix #3130 by changing

        private static bool Compat(AlignedArray a)
        {
            Contracts.AssertValue(a);
            Contracts.Assert(a.Size > 0);
            return a.CbAlign == Vector128Alignment;
        }

to

        private static bool Compat(AlignedArray a)
        {
            Contracts.AssertValue(a);
            Contracts.Assert(a.Size > 0);
            return a.CbAlign % Vector128Alignment == 0;
        }

@wschin wschin added the documentation Related to documentation of ML.NET label Mar 27, 2019
@wschin wschin self-assigned this Mar 27, 2019
@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #3124 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3124      +/-   ##
==========================================
- Coverage   72.62%   72.46%   -0.16%     
==========================================
  Files         807      802       -5     
  Lines      145080   144763     -317     
  Branches    16213    16199      -14     
==========================================
- Hits       105361   104904     -457     
- Misses      35300    35449     +149     
+ Partials     4419     4410       -9
Flag Coverage Δ
#Debug 72.46% <100%> (-0.16%) ⬇️
#production 68.02% <100%> (-0.15%) ⬇️
#test 88.84% <100%> (-0.09%) ⬇️
Impacted Files Coverage Δ
...ests/TrainerEstimators/MatrixFactorizationTests.cs 97.41% <100%> (+0.48%) ⬆️
...c/Microsoft.ML.CpuMath/CpuMathUtils.netstandard.cs 93.75% <100%> (ø) ⬆️
....ML.OnnxTransformer/DnnImageFeaturizerTransform.cs 0% <0%> (-95.24%) ⬇️
...ML.TestFramework/Attributes/OnnxTheoryAttribute.cs 0% <0%> (-83.34%) ⬇️
src/Microsoft.ML.OnnxTransformer/OnnxCatalog.cs 50% <0%> (-50%) ⬇️
src/Microsoft.ML.OnnxTransformer/OnnxTransform.cs 60.06% <0%> (-26.18%) ⬇️
src/Microsoft.ML.OnnxTransformer/OnnxUtils.cs 67.01% <0%> (-10.31%) ⬇️
...soft.ML.Data/DataView/DataViewConstructionUtils.cs 78.45% <0%> (-6.65%) ⬇️
src/Microsoft.ML.Data/Transforms/ColumnCopying.cs 83.49% <0%> (-1.95%) ⬇️
...OnnxTransformer.StaticPipe/OnnxStaticExtensions.cs
... and 7 more

@wschin wschin force-pushed the add-a-cpumath-doc-and-a-test branch from eccc434 to fda86d0 Compare March 28, 2019 17:59
@wschin wschin changed the title Add doc string to explain matrix-vector product's SSE code and a test [WIP] Add doc string to explain matrix-vector product's SSE code and a test Apr 4, 2019
@wschin wschin changed the title [WIP] Add doc string to explain matrix-vector product's SSE code and a test Add doc string to explain matrix-vector product's SSE code and a test Apr 9, 2019
@wschin wschin requested review from codemzs and shauheen April 9, 2019 23:25
[KeyType(_matrixColumnCount)]
public uint MatrixColumnIndex;
// Matrix row index starts from 0 and is at most _synthesizedMatrixRowCount.
// Contieuous=true means that all values from 0 to _synthesizedMatrixRowCount are allowed keys.
Copy link
Member

@codemzs codemzs Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contieuous [](start = 15, length = 10)

Contiguous? Continuous? seems to be like this few other places. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those Contieuouss are removed because they are not a part of the latest key type.


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

/// if <paramref name="a"/>'s alignment is ok and <see langword="false"/> otherwise.
/// </summary>
/// <param name="a">The vector being checked.</param>
/// <returns>Whether <see langword="true"/> is aligned well.</returns>
Copy link
Member

@codemzs codemzs Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 29, length = 22)

? #Resolved

Copy link
Member

@codemzs codemzs Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"< paramref name = "a" / > " ? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Thanks.


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

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@codemzs
Copy link
Member

codemzs commented Apr 9, 2019

Some minor clean up may be needed but I won't block on it and assume you will do it before checking in. #Resolved

@@ -34,6 +40,19 @@ private static bool Compat(AlignedArray a)
return q;
}

/// <summary>
/// Compute the product of (flattened because its type is <see cref="AlignedArray"/> instead of a matrix) matrix <paramref name="mat"/>
/// and a vector <paramref name="src"/>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    /// Compute the product of a flattened matrix <paramref name="mat"/> (flattened because its type is <see cref="AlignedArray"/>) 
    /// and a vector <paramref name="src"/>.

Maybe like this it is a bit easier to read.

Copy link
Member Author

@wschin wschin Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Thanks.

Copy link
Contributor

@artidoro artidoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@wschin wschin merged commit b6e602a into dotnet:master Apr 11, 2019
@wschin wschin deleted the add-a-cpumath-doc-and-a-test branch April 11, 2019 00:50
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Related to documentation of ML.NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CpuMath doesn't work as expected.
3 participants