Elementwise by kirillgarbar · Pull Request #46 · SparseLinearAlgebra/GraphBLAS-sharp

kirillgarbar · 2022-09-22T00:28:11Z

Proposed Changes

EWiseAdd renamed to elementwise
New elementwise for CSR
Vector types for GPU

Types of changes

What types of changes does your code introduce to GraphBLAS-sharp?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in 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.

Build and tests pass locally
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added necessary documentation (if appropriate)

gsvgit · 2022-09-22T05:03:38Z

-        let CSReWiseAdd =
-            CSRMatrix.eWiseAdd clContext opAdd workGroupSize
+        let CSRElementwise =
+            Elementwise.elementwise clContext opAdd workGroupSize


Why for COO name is COOMatrix.elementwise, but for CSR Elementwise.elemenwise, not CSRMatrix.elementwise?

gsvgit · 2022-09-22T05:04:54Z

-        let CSReWiseAdd =
-            CSRMatrix.eWiseAddAtLeastOne clContext opAdd workGroupSize
+        let CSRElementwise =
+            Elementwise.elementwiseAtLeastOne clContext opAdd workGroupSize


Why for COO name is COOMatrix.elementwiseAtLeastOne, but for CSR Elementwise.elementwiseAtLeastOne, not CSRMatrix.elementwiseAtLeastOne?

gsvgit · 2022-09-22T05:07:58Z

+
+    member this.Dispose(q) = (this :> IDeviceMemObject).Dispose(q)
+
+type DenseVector<'a> =


What the differrence from ClArray? Can we unify these types? Or make DenceVector (or clDenceVector) an extension of ClArray?

gsvgit · 2022-09-22T05:12:07Z

-          Backend.EwiseAdd.tests
-          Backend.EwiseAdd.tests2
-          //Backend.EwiseAdd.tests3
+          Backend.Elementwise.tests


Can you propose more user-friendly names than tests, tests2, tests3. What the difference betwee these tests?

artemgl · 2022-09-22T09:05:00Z

+
+                let row = globalID / workGroupSize
+
+                if row < rows then


It seems to be always true

artemgl · 2022-09-22T10:14:23Z

+                                + secondLocalOffset
+                                + i
+
+                            if resY = 1 || resX = 0 then


Code duplication can be eliminated by reducing this with one conditional expression

artemgl · 2022-09-22T11:43:30Z

+                //Init with zeroes
+                if i < numberOfRows then
+                    rowPointers.[i] <- 0
+                elif i = numberOfRows then


It's not necessary to handle this case separately because prefix sum is exclusive

artemgl · 2022-09-22T11:44:31Z

+        let merge = merge clContext workGroupSize
+
+        let preparePositions =
+            preparePositions clContext opAdd Utils.defaultWorkGroupSize


Why Utils.defaultWorkGroupSize but not workGroupSize ?

All steps of the elementwise except merge are working with the best performance for a relatively large workGroupSize regardless of the characteristics of the matrices. On the other hand, the choice of the best workGroupSize for merge may depend on matrices and it makes sense to vary it. It is possible to add a second parameter for workGroupSize, but since its choice is not so important, it was decided to apply the standard size.

artemgl · 2022-09-22T11:55:38Z

+                    let index = prefixSumArray.[i]
+
+                    resultColumns.[index] <- allColumns.[i]
+                    resultValues.[index] <- allValues.[i] @>


You could fill resultRows here as well and get result matrix in COO without further conversion. If it's not convenient to get result in COO, I think current conversion from COO to CSR is more optimal since it launches only one prefix sum algorithm

kirillgarbar · 2022-10-17T17:12:35Z

I think ClDenseVector should not inherit ClArray due to the fact that it is impossible to create ClDenseVector from ClArray because of ClArray's private constructor

gsvgit · 2022-10-17T17:55:40Z

@kirillgarbar Strange reason. May it be etter to modify ClArray? Or maybe ClDenseVector should aggregate ClArray. Anyway, I guess these two structures should be unified.

dpanfilyonok · 2022-10-22T18:32:24Z

+
+module ArraysExtensions =
+
+    type ClArray<'a> with


It does not respect the invariant that the type must be 'a option.

@dpanfilyonok Why must? In my opinion in may be Option<'t>, but may be not.

If not, we won't be able to determine if the value is 0 or not.

@dpanfilyonok But it is required only for ClVector, not for ClArray. But for ClVector it is specified that value is Option<'t>: https://github.com/YaccConstructor/GraphBLAS-sharp/pull/46/files#diff-fd9366b267db07ee1f0db1c7d4a56f1d2c91fee0be1da169aa171166eb294f87R131

But its not specified for ClDenseVector itself. Therefore, problems can arise if we want to use a dense vector by itself.

@dpanfilyonok What is Cl DenseVector? I cannot find declaration of this type.

artemgl · 2022-10-24T11:44:05Z

+    | COO
+    | Dense
+
+type COOVector<'a> =


I thought COO is for matrix format, and vector should better be something like SparseVector. But COO may probably be convenient for vector as well, I'm not sure

artemgl · 2022-10-24T11:44:28Z

+
+    member this.Dispose(q) = (this :> IDeviceMemObject).Dispose(q)
+
+type TuplesVector<'a> =


Why do we need this type? It looks exactly like COOVector

artemgl · 2022-10-24T11:46:25Z

        >> fun x -> x ||| (x >>> 16)
        >> fun x -> x + 1
+
+    let toHost (processor: MailboxProcessor<_>) (src: ClArray<_>) =


Does this already exist in ArraysExtentions?

artemgl · 2022-10-24T11:48:51Z

+open Microsoft.FSharp.Quotations
+
+module internal Elementwise =
+    let expandCompressedRowPointers (clContext: ClContext) workGroupSize =


Is this used? If not, it probably should be removed

artemgl · 2022-10-24T11:50:42Z

+
+            resultRows, resultColumns, resultValues, positions, resultLength
+
+    let getCompressedRowPointers (clContext: ClContext) workGroupSize =


As far as I know this is also not used

artemgl · 2022-10-24T12:01:03Z

+    type 'a ``[]`` with
+        member this.Size = this.Length
+
+        member this.ToString() =


Looks oddish for me. We may have an array that we don't treat as a dense vector

artemgl · 2022-10-24T12:15:09Z

+
+        member this.ToDevice(context: ClContext) = context.CreateClArray this
+
+    let FromArray (array: 'a [], isZero: 'a -> bool) =


Looks also oddish for me. It's not obvious that this function is related to dense vector

kirillgarbar added 24 commits July 21, 2022 15:01

CSR merge, no empty rows

f3ccf0b

CSR merge, empty row case

449e060

getUniqueBitmap CSR

79802c3

preparePositions CSR

c3ea68b

Set positions, elementwiseCSR

59f6c65

Add to fsproj

aceb130

Row sizes computing inside kernels

798562f

Work-group on each row (before rewriting merge)

c16714e

Work-group on each row, one block per row case is working

862f1e0

Row pointers computing

b8192f6

Work-group on each row, all test cases passed

a2a3b17

Brahma 2.0.1

247ba59

isRowEnd bitmap and merge cleanup

e2bfae9

DCSR to CSR conversion, EWise passing all tests except float

b3b245c

EWise fixes

e213590

Debug cleanup

39cd6b1

eWiseAdd for AtLeastOne CSR

9594bb7

EWiseAdd renamed to Elementwise

d457edd

Comments and minor fixes

9be27a0

CSR preparePositions fix

33348e0

Merge branch 'elementwise' into net5

d4ae372

Correct CSR format for elementwise and formatting

112271e

Benchmarks build fix

83f479a

Vector type moved to backend. Added GPU versions

3282bd6

kirillgarbar requested review from dpanfilyonok and gsvgit September 22, 2022 00:30

Old vector types removed

2747b77

gsvgit requested changes Sep 22, 2022

View reviewed changes

artemgl requested changes Sep 22, 2022

View reviewed changes

Build fix

3519203

kirillgarbar added 9 commits September 26, 2022 01:29

CSR elementwise moved to CSRMatrix module

239b1fe

New test names

d7f2435

Merge code refactored

80747b4

COO to CSR in elementwise instead of DCSR to CSR

8df0d50

elementwiseToCOO

6cd18ba

ClDenseVector inherits ClArray

9156991

Correct size member for sparse vectors

112a4ab

Catching InvalidBufferSize in tests

a6cf061

ClDenseVector struct instead of inheritance

22e6c3e

ClArray and Array extensions instead of wrapper-types for vectors

f12e605

gsvgit approved these changes Oct 22, 2022

View reviewed changes

dpanfilyonok requested changes Oct 22, 2022

View reviewed changes

Dispose and FromArray correction in ArraysExnentions

9de7da5

dpanfilyonok approved these changes Oct 24, 2022

View reviewed changes

artemgl reviewed Oct 24, 2022

View reviewed changes

kirillgarbar added 5 commits October 29, 2022 23:21

Elementwise setPositions fix

33d974e

Deleting unused and duplicated methods

36b3be4

SparseVector instead of COO, tuples deleted

b48705e

ArraysExnensions methods name changed

7affcd0

Build fix

5814867

artemgl approved these changes Oct 31, 2022

View reviewed changes

gsvgit merged commit 835e7bf into SparseLinearAlgebra:net5 Oct 31, 2022

kirillgarbar deleted the elementwise branch March 24, 2024 19:07


		member this.Dispose(q) = (this :> IDeviceMemObject).Dispose(q)

		type DenseVector<'a> =


		member this.Dispose(q) = (this :> IDeviceMemObject).Dispose(q)

		type TuplesVector<'a> =


		resultRows, resultColumns, resultValues, positions, resultLength

		let getCompressedRowPointers (clContext: ClContext) workGroupSize =


		member this.ToDevice(context: ClContext) = context.CreateClArray this

		let FromArray (array: 'a [], isZero: 'a -> bool) =

Conversation

kirillgarbar commented Sep 22, 2022

Proposed Changes

Types of changes

Checklist

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!

kirillgarbar commented Oct 17, 2022

Uh oh!

gsvgit commented Oct 17, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants