PageRank #88

kirillgarbar · 2023-10-16T20:01:48Z

Proposed Changes

Added PageRank algorithm and some minor changes to BFS and SSSP

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)

artemiipatov · 2023-10-21T18:52:45Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+        let one =
+            <@ fun (x: float32 option) (_: int option) ->
+                let mutable res = 0
+
+                match x with
+                | Some _ -> res <- 1
+                | None -> ()
+
+                if res = 0 then None else Some res @>


Can we make it shorter like this?

Suggested change

let one =

<@ fun (x: float32 option) (_: int option) ->

let mutable res = 0

match x with

| Some _ -> res <- 1

| None -> ()

if res = 0 then None else Some res @>

let one =

<@ fun (x: float32 option) (_: int option) ->

if x.IsNone then None else Some 1 @>

IsNone is not supported in Brahma

Can you add an issue?

We can implement this with quotations. Is it worth doing this in Brahma?

@IgorErin I'm not sure that I see an alternative way rather that to support IsNone translation in Brahma.

@gsvgit I'm thinking about a standard library for quotas in Brahma. Here you can, for example, iter. And you won't have to hardcode into the compiler.

artemiipatov · 2023-10-21T18:54:54Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+            <@ fun (x: float32 option) y ->
+                let mutable res = 0.0f
+
+                match x, y with
+                | Some _, Some y -> res <- alpha / (float32 y)
+                | Some _, None -> ()
+                | None, Some _ -> ()
+                | None, None -> ()
+
+                if res = 0.0f then None else Some res @>


Suggested change

<@ fun (x: float32 option) y ->

let mutable res = 0.0f

match x, y with

| Some _, Some y -> res <- alpha / (float32 y)

| Some _, None -> ()

| None, Some _ -> ()

| None, None -> ()

if res = 0.0f then None else Some res @>

<@ fun (x: float32 option) y ->

let mutable res = None

match x, y with

| Some _, Some y -> res <- Some (alpha / (float32 y))

| _ -> ()

res @>

artemiipatov · 2023-10-21T18:58:32Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+    let alpha = 0.85f
+    let accuracy = 0.00000001f
+
+    let countOutDegree (clContext: ClContext) workGroupSize =


Probably it is worth making these constants and functions private. They are specific for current module, user does not want to see them.

artemiipatov · 2023-10-21T19:00:50Z

src/GraphBLAS-sharp.Backend/Quotes/Arithmetic.fs

@@ -244,3 +244,16 @@ module ArithmeticOperations =
            | Some x, None -> Some x
            | None, Some y -> Some y
            | _ -> None @>
+
+    //PageRank specific
+    let minusAndSquare =


Probably name squareOfDifference sounds more clear?

artemiipatov · 2023-10-21T19:01:54Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+        let alpha = 0.85f
+        let accuracy = 0.00000001f


Constants are repeating.

artemiipatov · 2023-10-21T19:02:34Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+
+    let prepareMatrix (clContext: ClContext) workGroupSize =
+
+        let alpha = 0.85f


artemiipatov · 2023-10-21T19:35:36Z

tests/GraphBLAS-sharp.Tests/Backend/Algorithms/PageRank.fs

+open GraphBLAS.FSharp.Objects
+open GraphBLAS.FSharp.Objects.MatrixExtensions
+
+let testFixtures (testContext: TestContext) =


Weak tests.

artemiipatov · 2023-10-21T20:06:03Z

src/GraphBLAS-sharp.Backend/Algorithms/Algorithms.fs

+        let run = PageRank.run
+
+        let prepareMatrix = PageRank.prepareMatrix


I think that prepareMatrix should at least have some clarifying comments. Also, to run page rank algorithm user always has to call prepareMatrix first. And I don't think he wants to build a lego house every time.

Always calling prepareMatrix before algorithm isn't necessary. User might already have matrix in correct format, for example in benchmarks. User may also want to obtain the prepared matrix and use it elsewhere.

How do we inform the user about this need? What guarantees are there that this condition will be met?

We cannot afford to set such semantic traps at any level of abstraction.

If a precondition arises, it must be expressed in types so that the user is not able to pass an unprepared matrix.

Otherwise, any mention of functional style should be removed from the title of our work "graphblas in functional style".

I believe we do not need to inform user about it because it is not our goal to provide PageRank implementation but to provide enough building blocks to implement it.
This implementation is nothing more but example and test of our api.
It is required to know how PageRank works to implement it using our library, so I think we should not care a lot about specific graph algorithms such as BFS or PageRank inside our library

@gsvgit What do you think?

Ready-to-use garph analysis algorithms should be a part of library. It is the next (possibli highest) leyer of our solution (look at LaGraph). So, implementations of BFS, PAgeRank and other algorithms should be as clear and documented, as possible. Anyway, even basic implmenetation should be a clear anough to be a starting point for those who want ot develop new algorithms using provieed building blocks.

Regarding types. I agree with @IgorErin: if we can express something using types, we should do it. Ofcourse we should find the best way to do it.

artemiipatov · 2023-10-21T20:13:20Z

src/GraphBLAS-sharp.Backend/Algorithms/Algorithms.fs

@@ -14,3 +14,8 @@ module Algorithms =

    module SSSP =
        let singleSource = SSSP.run


Suggested change

let singleSource = SSSP.run

let run = SSSP.run

kirillgarbar · 2023-12-02T15:43:39Z

Added PageRankMatrix type as single case union type which is produced by prepareMatrix

gsvgit · 2023-12-07T12:21:18Z

src/GraphBLAS-sharp.Backend/Algorithms/BFS.fs

@@ -67,14 +67,14 @@ module internal BFS =
            levels

    let singleSourceSparse
-        (add: Expr<bool option -> bool option -> bool option>)
-        (mul: Expr<bool option -> bool option -> bool option>)
+        (add: Expr<int option -> int option -> int option>)


Why we should use int instead of bool?

The only reason is that spla and GraphBLAST use int and float matrices on BFS and comparison will be more fair. But now I think it is a strange reason so I'll change that to bool again

artemiipatov · 2023-12-10T20:40:08Z

src/GraphBLAS-sharp.Backend/Algorithms/Algorithms.fs

@@ -19,3 +19,8 @@ module Algorithms =

    module SSSP =
        let run = SSSP.run
+
+    module PageRank =
+        let run = PageRank.run


I think documentation is needed. At least we should say that matrix has to be prepared first by calling prepareMatrix method.

IgorErin · 2023-12-10T20:24:20Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/PageRank.fs

+    let mutable matrixPrepared = Unchecked.defaultof<PageRankMatrix<float32>>
+    let mutable matrixHost = Unchecked.defaultof<_>
+
+    let accuracy = 0.00000001f


There are predefined constants in Expecto.

Still, why is it necessary separately? Perhaps we need to create a module with constants and actions on them in order to avoid mistakes in the future?

Abstract types could also be used here, so that they could be used only with the help of this most magical module.

IgorErin · 2023-12-10T20:27:32Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/PageRank.fs

+        Benchmarks.InputMatrixProviderBuilder "BFSBenchmarks.txt"
+
+    [<GlobalSetup>]
+    override this.GlobalSetup() =


I've always thought about it. Don't you need to make the preparatory methods blocking? Then is it possible to avoid blocking in the [<Benchmark>] itself?

It is a good idea to block after preparotory methods in case some of them are not blocking by itself, but blocking in the [<Benchmark>] is still necessary to wait for all kernels to execute.

IgorErin · 2023-12-10T21:00:37Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+        let one =
+            <@ fun (x: float32 option) (_: int option) ->
+                let mutable res = 0
+
+                match x with
+                | Some _ -> res <- 1
+                | None -> ()
+
+                if res = 0 then None else Some res @>


@gsvgit I'm thinking about a standard library for quotas in Brahma. Here you can, for example, iter. And you won't have to hardcode into the compiler.

IgorErin · 2023-12-10T21:14:43Z

src/GraphBLAS-sharp.Backend/Objects/Matrix.fs

+/// <summary>
+/// Represents an abstraction over matrix, which is converted to correct format for PageRank algorithm
+/// </summary>
+type PageRankMatrix<'a when 'a: struct> =


Great. But it needs to be even stronger. Abstract types are needed here. Like here in the first example.

https://okmij.org/ftp/Computation/lightweight-static-guarantees.html

I was told that this can be done using signatures. The idea is that we should not allow anyone to use this type unless they have passed some kind of verification. It seems to me that this also applies to disjoint matrices, @artemiipatov .

Of course, this can be disabled during benchmarking.

In the same way, we will add another functional and at the same time zero abstraction. This is exactly what is needed on FHPNC

Signatures are not working?

IgorErin · 2023-12-10T21:16:02Z

tests/GraphBLAS-sharp.Tests/Backend/Algorithms/PageRank.fs

+open GraphBLAS.FSharp.Objects
+
+let private alpha = 0.85f
+let private accuracy = 0.00001f


Expecto have some constants like that. We use they in tests

IgorErin

I haven't tried them in F#, is it possible to use abstract types in the language?

IgorErin · 2023-12-18T13:10:34Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/BFS.fs

@@ -113,10 +113,12 @@ type WithoutTransferBenchmark<'elem when 'elem : struct>(
    override this.GlobalSetup() =
        this.ReadMatrix()
        this.LoadMatrixToGPU()
+        this.Processor.PostAndReply(Msg.MsgNotifyMe)


How much is it necessary to duplicate this? Is it possible to make an extension?

I tried to make extension for specialization of generic MailboxProcessor and failed, as far as I know it is possible only for static members and static method would look not much better than this

Ok. Seems like that. Than maybe just

module Queue = let block (q: MailboxProcessor<Msg>) = q.PostAndReply(Msg.MsgNotifyMe)

in GraphBlas-sharp.Backend.Objects

I was told that this may work.

[<System.Runtime.CompilerServices.Extension>] // На F# 8 можно убрать. type MailboxProcessorExts = [<System.Runtime.CompilerServices.Extension>] static member Block(this : MailboxProcessor<int>) = this.Post 42

We can choose!

IgorErin · 2023-12-18T13:18:29Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/PageRank.fs

+    let mutable matrixPrepared = Unchecked.defaultof<PageRankMatrix<float32>>
+    let mutable matrixHost = Unchecked.defaultof<_>
+
+    let accuracy = 0.00000001f


Still, why is it necessary separately? Perhaps we need to create a module with constants and actions on them in order to avoid mistakes in the future?

Abstract types could also be used here, so that they could be used only with the help of this most magical module.

IgorErin · 2023-12-18T13:23:52Z

src/GraphBLAS-sharp.Backend/Objects/Matrix.fs

+/// <summary>
+/// Represents an abstraction over matrix, which is converted to correct format for PageRank algorithm
+/// </summary>
+type PageRankMatrix<'a when 'a: struct> =


Signatures are not working?

IgorErin · 2023-12-18T13:25:44Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+            let addition =
+                create queue DeviceOnly vertexCount Dense (Some((1.0f - alpha) / (float32 vertexCount)))
+
+            let mutable error = accuracy + 0.1f


I'm all about a separate module for constants. Then it would be possible to take out actions like reciprocal and the like. Then, perhaps such actions will look more meaningful.

Or use predefined ones. Or comment on it. It's just that the number of magic constants scares me.

It still confuses me. And float arithmetic 3 line above too!

kirillgarbar · 2023-12-21T22:38:47Z

Added signature file for PageRank module so it is now impossible to do something withPageRankMatrix from outside.
Had to made module Backend.PageRank not internal because type PageRankMatrix was not visible from Algorithms.PageRank module, all members except for type were made internal instead.
Also, I was forced to do implementation of type as discriminated union due to compiler arguing about type abbreviation.

IgorErin · 2023-12-22T08:38:19Z

@gsvgit Can we disable document assembly on Windows? This red mark is disorienting

IgorErin

Great. There's nothing left

IgorErin · 2023-12-22T09:10:02Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/BFS.fs

@@ -113,10 +113,12 @@ type WithoutTransferBenchmark<'elem when 'elem : struct>(
    override this.GlobalSetup() =
        this.ReadMatrix()
        this.LoadMatrixToGPU()
+        this.Processor.PostAndReply(Msg.MsgNotifyMe)


Ok. Seems like that. Than maybe just

module Queue = let block (q: MailboxProcessor<Msg>) = q.PostAndReply(Msg.MsgNotifyMe)

in GraphBlas-sharp.Backend.Objects

IgorErin · 2023-12-22T09:12:21Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+            let addition =
+                create queue DeviceOnly vertexCount Dense (Some((1.0f - alpha) / (float32 vertexCount)))
+
+            let mutable error = accuracy + 0.1f


It still confuses me. And float arithmetic 3 line above too!

IgorErin · 2023-12-22T09:18:12Z

benchmarks/GraphBLAS-sharp.Benchmarks/Algorithms/BFS.fs

@@ -113,10 +113,12 @@ type WithoutTransferBenchmark<'elem when 'elem : struct>(
    override this.GlobalSetup() =
        this.ReadMatrix()
        this.LoadMatrixToGPU()
+        this.Processor.PostAndReply(Msg.MsgNotifyMe)


I was told that this may work.

[<System.Runtime.CompilerServices.Extension>] // На F# 8 можно убрать. type MailboxProcessorExts = [<System.Runtime.CompilerServices.Extension>] static member Block(this : MailboxProcessor<int>) = this.Post 42

We can choose!

IgorErin · 2023-12-22T09:20:10Z

src/GraphBLAS-sharp.Backend/Constants/Constants.fs

+        /// </summary>
+        let alpha = 0.85f
+
+    module Common =


I don't know how convenient it is to move something that is used only in tests here. But okay

Previously it was in Backend.Utils and used in SpMV for the reason that I forgot. I imagine we can use it somewhere if we know for sure that this is the optimal size for the algorithm

It seems to me that the optimal size should be in benchmarks. And there will be several of them, depending on the hardware and other things. So that it is not just optimal, but everyone can check it relatively quickly in a particular case, just by cloning the repository and writing a couple of commands. But that's sometime later.

If this was hardcoded in SpMV, then shouldn't we make a separate submodule for SpMV in the constants module? Just like you did for PageRank?

It is better to use a constant from the SpMV module in tests than to use a constant from tests in SpMV.

His whole idea is just not to forget where the constants came from or what actions to take with them. In my opinion, putting it in a separate module with an obvious name while providing comments with the reason is very good.

IgorErin · 2023-12-22T09:24:03Z

src/GraphBLAS-sharp.Backend/Algorithms/PageRank.fs

+open GraphBLAS.FSharp.Objects.ArraysExtensions
+open GraphBLAS.FSharp.Objects.ClCellExtensions
+
+module PageRank =


[<RequierQulifierAccess>] for all API modules

kirillgarbar · 2023-12-22T20:25:27Z

Don't know why I can't reply to a comment about let mutable error.
This variable is created in such way that while cycle will be entered, looks pretty clear to me.
Also, explaining why vectors above are created in such way in a few lines of comments instead of article about PageRank is not possible, implementing PageRank implies that person would understand why such values are used.
Only reasonable thing to do in this case might be a link to the article in the comment

IgorErin · 2023-12-23T09:47:54Z

Yes, then we can stop at the comments

kirillgarbar requested a review from artemiipatov October 16, 2023 23:02

artemiipatov suggested changes Oct 21, 2023

View reviewed changes

PageRankMatrix type

faf242c

gsvgit reviewed Dec 7, 2023

View reviewed changes

kirillgarbar added 4 commits December 10, 2023 16:45

BFS on bool

b278b7a

Merge remote-tracking branch 'YaccConstructor/dev' into dev

ebdb250

Fix merge

6f3819b

Fix ZeroCreate test

1ed601c

kirillgarbar requested a review from gsvgit December 10, 2023 15:56

gsvgit requested review from IgorErin and artemiipatov December 10, 2023 18:26

artemiipatov reviewed Dec 10, 2023

View reviewed changes

IgorErin suggested changes Dec 10, 2023

View reviewed changes

kirillgarbar added 3 commits December 17, 2023 19:23

Constants for test

858cd82

Docs for PageRank

0826562

Block after setup and cleanup in benchmarks

b82fe68

gsvgit requested a review from IgorErin December 18, 2023 09:20

IgorErin reviewed Dec 18, 2023

View reviewed changes

Constants module and signature for PageRank module

c8f3042

IgorErin reviewed Dec 22, 2023

View reviewed changes

kirillgarbar added 2 commits December 22, 2023 23:12

MailboxProcessor extensions

0ab7361

Module qualifiers

97b6db4

kirillgarbar and others added 2 commits December 23, 2023 21:20

Add pagerank link

f671a2e

Restart CI attempt

6de410f

gsvgit merged commit 7fe8ff3 into SparseLinearAlgebra:dev Mar 18, 2024
4 of 5 checks passed


		let prepareMatrix (clContext: ClContext) workGroupSize =

		let alpha = 0.85f

		let run = PageRank.run

		let prepareMatrix = PageRank.prepareMatrix

		@@ -14,3 +14,8 @@ module Algorithms =

		module SSSP =
		let singleSource = SSSP.run

PageRank #88

PageRank #88

Conversation

kirillgarbar commented Oct 16, 2023

Proposed Changes

Types of changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorErin Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillgarbar commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorErin Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorErin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirillgarbar commented Dec 21, 2023 • edited Loading

IgorErin commented Dec 22, 2023

IgorErin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorErin Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

kirillgarbar commented Dec 22, 2023

IgorErin commented Dec 23, 2023

IgorErin Dec 10, 2023 •

edited

Loading

IgorErin Dec 10, 2023 •

edited

Loading

kirillgarbar commented Dec 21, 2023 •

edited

Loading

IgorErin Dec 22, 2023 •

edited

Loading