Kronecker #79
Merged
Kronecker #79
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gsvgit
reviewed
May 11, 2023
| | Some _ -> resultBitmap.[0] <- newItem | ||
| | _ -> () | ||
|
|
||
| else if (gid - 1) < valuesLength then |
| | ClMatrix.CSR m1, ClMatrix.CSR m2 -> | ||
| let result = run queue allocationFlag m1 m2 | ||
| Option.map ClMatrix.COO result | ||
| | _ -> failwith "Matrix formats are not matching" |
There was a problem hiding this comment.
Can you provide more information in the message to make it more useful?
| let mutable res = 0 | ||
|
|
||
| let xInt = | ||
| match x with |
There was a problem hiding this comment.
Can you add a comment to clarify why these conversions are needed?
Should it be reported as a quotation evaluator bug?
There was a problem hiding this comment.
I've reported the issue.
fsprojects/FSharp.Quotations.Evaluator#45
| @@ -73,7 +125,7 @@ module ArithmeticOperations = | |||
| mkUnaryOp zero <@ fun x -> x + constant @> | |||
|
|
|||
| let intSumOption = mkNumericSum 0 | |||
| let byteSumOption = mkNumericSum 0uy | |||
There was a problem hiding this comment.
So... Finally, what about bytes?
| let create = create clContext workGroupSize | ||
|
|
||
| let init = | ||
| init <@ fun x -> x @> clContext workGroupSize |
There was a problem hiding this comment.
Can it be replaced with <@ id @> ?
IgorErin
reviewed
May 12, 2023
| @@ -53,6 +53,7 @@ | |||
| <Compile Include="Backend/Matrix/Merge.fs" /> | |||
| <Compile Include="Backend/Matrix/ExpandRows.fs" /> | |||
| <Compile Include="Backend/Matrix/SubRows.fs" /> | |||
| <Compile Include="Backend\Matrix\Kronecker.fs" /> | |||
|
|
||
| let createGeneralTest (context: ClContext) (processor: MailboxProcessor<Msg>) (zero: 'a) isEqual op opQ testName = | ||
|
|
||
| let kronecker = |
There was a problem hiding this comment.
Why is the pipeline not used here?
| createGeneralTest context queue false (=) (&&) ArithmeticOperations.boolMulOption "mul" | ||
| createGeneralTest context queue false (=) (||) ArithmeticOperations.boolSumOption "sum" | ||
|
|
||
| createGeneralTest context queue 0 (=) (*) ArithmeticOperations.intMulOption "mul" |
There was a problem hiding this comment.
Can we pass the testContext as a single parameter in order to reduce their number?
| kroneckerFun processor ClContext.HostInterop m1 m2 | ||
|
|
||
| let actual = | ||
| Option.bind (fun (m: ClMatrix<'a>) -> m.ToHost processor |> Some) result |
There was a problem hiding this comment.
Why can't Option.map be used here?
| |> Some) | ||
| | None -> | ||
| opOnHost value None | ||
| |> Option.bind |
| let result = | ||
| setPositions queue allocationMode values indices bitmap | ||
|
|
||
| queue.Post(Msg.CreateFreeMsg<_>(indices)) |
| let result = | ||
| setPositions queue allocationMode rows columns values bitmap | ||
|
|
||
| queue.Post(Msg.CreateFreeMsg<_>(bitmap)) |
| let result = | ||
| mapAll queue allocationMode size matrixZero matrixLeft matrixRight | ||
|
|
||
| match matrixZero with |
| let startIndex = | ||
| insertNonZero queue rowsEdges matrixRight matrixLeft.Values leftColumns resultMatrix | ||
|
|
||
| match matrixZero with |
IgorErin
approved these changes
May 13, 2023
gsvgit
approved these changes
May 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
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...