Kronecker #73
Kronecker #73
Conversation
| let newColumnIndices = | ||
| mapWithValueClArray queue allocationMode columnOffset m.Columns | ||
|
|
||
| queue.Post(Msg.CreateFreeMsg<_>(rowOffset)) |
There was a problem hiding this comment.
Why don't you use the Free extension method?
| let leftMatrixCols = matrixLeft.Columns.ToHost queue | ||
| let leftMatrixVals = matrixLeft.Values.ToHost queue | ||
|
|
||
| for row in 0 .. (matrixLeft.RowCount - 1) do |
There was a problem hiding this comment.
It may be worth dividing the logical parts of this function into separate functions
| queue.Post(Msg.CreateFreeMsg<_>(rows)) | ||
| queue.Post(Msg.CreateFreeMsg<_>(columns)) | ||
|
|
||
| match result with |
There was a problem hiding this comment.
Why Option.bind cannot be used here
| @@ -8,7 +8,7 @@ open GraphBLAS.FSharp.Backend.Objects.ClCell | |||
| module Common = | |||
| ///<param name="clContext">.</param> | |||
| ///<param name="workGroupSize">Should be a power of 2 and greater than 1.</param> | |||
| let setPositions<'a when 'a: struct> (clContext: ClContext) workGroupSize = | |||
| let setPositionsUnsafe<'a when 'a: struct> (clContext: ClContext) workGroupSize = | |||
There was a problem hiding this comment.
Should we rename both functions? Why safe? Wouldn't it be more appropriate to use option here
| HostPrimitives.array2DMultiplication zero opMul opAdd leftArray rightArray | ||
| |> fun array -> Utils.createMatrixFromArray2D COO array (isEqual zero) | ||
|
|
||
| if matrixExpected.NNZ > 0 then |
There was a problem hiding this comment.
Can we check the conditions in one place in order to avoid nesting?
| leftMatrixCols.[firstElementIndex + offset] | ||
|
|
||
| let numberOfZeroElements = | ||
| match offset with |
There was a problem hiding this comment.
Perhaps it is better to use if here?
| | _ -> | ||
| currentColumn | ||
| - leftMatrixCols.[firstElementIndex + offset - 1] | ||
| - 1 |
There was a problem hiding this comment.
Why can't we count the number of zero elements using Array.pairwise or something like that? Then use Array.map...
| RowCount = matrixLeft.RowCount * matrixRight.RowCount | ||
| ColumnCount = matrixLeft.ColumnCount * matrixRight.ColumnCount } | ||
|
|
||
| let run<'a, 'b, 'c when 'a: struct and 'b: struct and 'c: struct and 'c: equality> |
There was a problem hiding this comment.
Why is it necessary to explicitly pair the conversion function? It seems that at the same time we lose in modularity
|
|
||
| let kernel = clContext.Compile <| preparePositions op | ||
|
|
||
| fun (processor: MailboxProcessor<_>) (operand: ClCell<'a option>) rowCount columnCount (values: ClArray<'b>) (rowPointers: ClArray<int>) (columns: ClArray<int>) -> |
There was a problem hiding this comment.
Is it necessary to pass all the fields of the matrix one by one? Is it possible to pass the entire matrix at once?
| let result = | ||
| runCSR queue allocationFlag matrix1 matrix2 | ||
|
|
||
| match result with |
There was a problem hiding this comment.
It is assumed that the result cannot be a null matrix?
| let queue = testContext.Queue | ||
| queue.Error.Add(fun e -> failwithf "%A" e) | ||
|
|
||
| createGeneralTest context queue false (=) (&&) ArithmeticOperations.boolMulOption "bool mul" |
There was a problem hiding this comment.
Is it worth passing the type of test data if it is already present in the name of the test?
| let localID = ndRange.LocalID0 | ||
|
|
||
| if localID < 2 then | ||
| let x = localID * (workGroupSize - 1) + i - 1 |
There was a problem hiding this comment.
In case of first element x = -1. Is it normal?
Proposed Changes
Types of changes
What types of changes does your code introduce to GraphBLAS-sharp?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...