Matrix.map #68
Matrix.map #68
Conversation
IgorErin
left a comment
IgorErin
left a comment
There was a problem hiding this comment.
Could we generalize some functions and logically separate them?
| (uint64 rowIndex <<< 32) ||| (uint64 columnIndex) | ||
|
|
||
| let value = | ||
| (%Map2.binSearch) valuesLength index rowPointers columns values |
There was a problem hiding this comment.
Given the popularity of this function, could you separate it into a separate module.
| open GraphBLAS.FSharp.Backend.Objects.ClContext | ||
|
|
||
| module Map = | ||
| let binSearch<'a> = |
There was a problem hiding this comment.
It looks like an optimization of the original function. Could we use it in Map2 too? Could we move this function to a separate module as well?
| let q = case.TestContext.Queue | ||
| q.Error.Add(fun e -> failwithf "%A" e) | ||
|
|
||
| let addFloat64Q = |
There was a problem hiding this comment.
Can we separate these functions into a separate module and make them more generalized in order to reduce duplication?
There was a problem hiding this comment.
I guess, these functions can be generalized and placed here
| open GraphBLAS.FSharp.Backend.Objects.ClMatrix | ||
| open GraphBLAS.FSharp.Backend.Objects.ClContext | ||
|
|
||
| module Map = |
There was a problem hiding this comment.
Could you make these module internal according to Map2
| workGroupSize | ||
| = | ||
|
|
||
| let elementwiseToCOO = runToCOO clContext opAdd workGroupSize |
There was a problem hiding this comment.
As far as I know, functions of this kind should be called map*. This name can be confusing
| <Compile Include="Quotes/PreparePositions.fs" /> | ||
| <Compile Include="Quotes/Predicates.fs" /> | ||
| <Compile Include="Quotes/Map.fs" /> | ||
| <Compile Include="Quotes\BinSearch.fs" /> |
| open GraphBLAS.FSharp.Backend.Objects | ||
|
|
||
| module ArithmeticOperations = | ||
| let inline mkOpWithConst zero op constant = |
There was a problem hiding this comment.
I think we should have two versions because non-commutative operations exist.
| open Brahma.FSharp | ||
|
|
||
| module BinSearch = | ||
| let searchInRange<'a> = |
| let q = case.TestContext.Queue | ||
| q.Error.Add(fun e -> failwithf "%A" e) | ||
|
|
||
| let mulFloat64Q = |
There was a problem hiding this comment.
Can we predefine these functions?
| if Utils.isFloat64Available context.ClDevice then | ||
| createTestMap case 0.0 ((*) 10.0) Utils.floatIsEqual (ArithmeticOperations.mulLeftConst 0.0 10.0) Matrix.map | ||
|
|
||
| createTestMap case 0.0f ((*) 10.0f) Utils.float32IsEqual (ArithmeticOperations.mulLeftConst 0.0f 10.0f) Matrix.map |
There was a problem hiding this comment.
We can pass zero, constant and both functions once to the createTestMap. So as not to write them twice
| /// <remarks> | ||
| /// Position is uint64 and it should be written in such format: first 32 bits is row, second 32 bits is column. | ||
| /// </remarks> | ||
| let searchCOO<'a> = |
There was a problem hiding this comment.
Maybe we'll call this function byKey2D, since the word search is present in the module name.
| [ let context = case.TestContext.ClContext | ||
| let q = case.TestContext.Queue | ||
| q.Error.Add(fun e -> failwithf "%A" e) | ||
|
|
There was a problem hiding this comment.
Integers must be supported by the operation. Could you add tests to this
|
Can you resolve conflicts caused by merging of previous request? |
3 participants
Proposed Changes
Matrix.map
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...