search

JuliaSparse / SuiteSparseGraphBLAS.jl

Sparse, General Linear Algebra for Graphs!
MIT License
102 stars 17 forks source link

Add matrix, vector and descriptor interface #4

Closed abhinavmehndiratta closed 5 years ago

abhinavmehndiratta commented 5 years ago

@simonschoelly @jpfairbanks

simonschoelly commented 5 years ago

This was merged a bit prematurely, but maybe it was necessary so you could use these changes here to continue working? In any case, I think there are still some open thinks here.

abhinavmehndiratta commented 5 years ago

This was merged a bit prematurely, but maybe it was necessary so you could use these changes here to continue working? In any case, I think there are still some open thinks here.

Hey, @simonschoelly I merged it because I needed to write some tests in order to proceed but if you have some ideas on improving the interface, I can open another PR or you can comment here itself.

simonschoelly commented 5 years ago

I think it might actually be possible to merge an issue, without closing one, so maybe check if you can reopen this one. Otherwise I think I will still comment here.

simonschoelly commented 5 years ago

So one think I noticed here, is that you are using the error method. I think it would be better to throw a more specify error, either one that is already in Base (maybe compare to what these methods throw for other matrices) or by creating your own Exception types.

You can read about that here: https://docs.julialang.org/en/v1/manual/control-flow/#Exception-Handling-1

simonschoelly commented 5 years ago

So have we decided, that we automatically make these matrices/vectors here one-based? We should consider, that this might lead to some confusion when someone mixes the more abstract methods with the more concrete ones.

Not that I am completely opposed to that, but we could also consider the fact, that there are some methods for dealing with non-1 based matrices in Julia Base.

simonschoelly commented 5 years ago

I see that GrB_Matrix is not a subtype of AbstractMatrix or AbstractSparseMatrix. Would it be a good idea to make this so? Then we might gain a lot of functionality for free. We don't have multiple inheritance though, so that would maybe influence the type hierarchy that you already created. You could maybe then use traits for your own functions then.

Another option would be to create a wrapper type, for example (might differ a bit in practice)

struct AsJuliaMatrix{T} <: AbstractMatrix{T}
   M::GrBMatrix
end

and then define all the Julia matrix functions on this instead.