Skip to content

COO operations#4

Closed
artemgl wants to merge 6 commits intoSparseLinearAlgebra:masterfrom
artemgl:coo-operations
Closed

COO operations#4
artemgl wants to merge 6 commits intoSparseLinearAlgebra:masterfrom
artemgl:coo-operations

Conversation

@artemgl
Copy link
Copy Markdown
Collaborator

@artemgl artemgl commented Nov 17, 2020

Proposed Changes

Added matrix in COO format and sparse vector. Changed mask representation into class, added regular one- and two-dimensional masks in COO format.

Types of changes

  • 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

  • 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)

Further comments

@@ -0,0 +1,14 @@
namespace GraphBLAS.FSharp

type COORegularMask2D(rows: array<int>, columns: array<int>, rowCount: int, columnCount: int) =
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

According to the F# style guide, int[] is preferred rather than int array or array<int>.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add lint tool to automate code style checking?

abstract Item: Mask2D<'a> -> Matrix<'a> with get, set
abstract Item: Mask1D<'a> * int -> Vector<'a> with get, set
abstract Item: int * Mask1D<'a> -> Vector<'a> with get, set
abstract Item: Mask2D -> Matrix<'a> with get, set
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mask is an optional parameter (Some or None), so we need to use Mask2D option instead of simple Mask2D


let matrixColumnCount = matrix.ColumnCount

let plus = !> semiring.PlusMonoid.Append
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here we can use single case DU deconstruction like this let (BinaryOp plus) = context.PlusMonoid.Append

type COOVector<'a when 'a : struct and 'a : equality>(dyads: array<'a * int>, length: int) =
inherit Vector<'a>()

let mutable dyads = Array.distinct dyads
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In GraphBLAS API there is an additional parameter dup, which responsible for processing duplicates in input pairs.

// | Complemented2D of Matrix<'a>
// | None

and [<AbstractClass>] Mask1D() =
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why should these classes be abstract? I thought we were going to represent a single implementation for the mask, and it would be based on different implementations of the sparse matrix and vector. So, it seems to me, that using a mask should look like this: currentVisited.VxmInplace matrix (Mask1D levels) BooleanSemiring.OrAnd. This question requires discussion. Ultimately, in C API the raw matrix or vector is passed to the operations as mask.

Copy link
Copy Markdown
Collaborator

@dpanfilyonok dpanfilyonok Nov 18, 2020

Choose a reason for hiding this comment

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

If we will only use sparse representations of the matrix and the vector (or the dense representation of the vector, but with an explicit indication of the semiring inside), will there be problems with the implementation of the mask as DU? (Except for the inability to overload indexers).

@@ -0,0 +1,14 @@
namespace GraphBLAS.FSharp

type COORegularMask2D(rows: array<int>, columns: array<int>, rowCount: int, columnCount: int) =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add lint tool to automate code style checking?

@@ -0,0 +1,113 @@
namespace GraphBLAS.FSharp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why CSR matrix and related stuff are also added in this PR? Looks like it is a reason of the merge conflict.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess you should pull the current master to your branch first.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That stuff is added because it uses entity that I changed (mask). I had to correct this files to avoid problems with them. But to be honest I can't understand why these corrections are recognized as conflicts, I'll try to pull master

@artemgl artemgl closed this Nov 26, 2020
gsvgit pushed a commit that referenced this pull request Feb 19, 2021
…tion

Coo element wise addition optimization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants