Update set method for sparse to only add non-zero elements

[Lundez]

I think it makes sense to disregard irrelevant values for the sparse matrices. I first noticed this issue when using another library (kmath) which has a buildMatrix method they yet have to specialize for sparse structures.
This buildMatrix would be much better off if the set method never applied "0" elements. They risk of doubling the array-size way larger than required would still exist, but at least we wouldn't make sure that it happens 😉

The best approach forward is for kmath to better support sparse structures, but this step in the right direction would be helpful for not only kmath-user but also ejml ones 😄

Before submitting this PR, please make sure:

• [-] Your code is covered by tests (something about memory breaking, not sure what...)
• You ran ./gradlew spotlessJavaApply

[lessthanoptimal]

@Lundez thanks for the PR! Going to need to put some thought into this and make sure there are no unintended consequences. This might have also come up once before but I can't seem to find the PR or discussion. For example a potential negative is that, maybe someone wants to pre-initialize the sparse structure and now they need to jump through hoops and this is an unexpected behavior. In my personal real-world applications this isn't useful since I'm not assigning values unless it's non-zero (e.g Jacobian of a large sparse system). As an example of untended consequences, libraries which mark matrices as transposed instead of transposing them to save time/memory basically just end up with much more complex slower code and dubious memory saving.

Are there any more established sparse linear algebra libraries which have this as their default behavior? E.g. SuiteSparse

@FlorentinD @szarnyasg Any thoughts on this as well?

[szarnyasg]

@lessthanoptimal I think this is related to an earlier discussion in the SuiteSparse:GraphBLAS repository: GraphBLAS/LAGraph#28

It's quite a lengthy one, so the issue of explicit zeros is not a trivial one. SuiteSparse:GraphBLAS can yield explicit zeros in some cases and there are also ways to drop them (e.g. with GxB_select).

[lessthanoptimal]

I don't think this is going to work as intended. Once a non-zero element is already in the graph structure you want to assign it the new value no matter what. If the element is not in the graph structure and the value being assigned is zero you can safely not add it.

[lessthanoptimal]

Already in the graph structure so you want to update the value.

[lessthanoptimal]

not in the graph structure so you could check to see if it's zero here and not add it.

[lessthanoptimal]

see comments for triplet below.

[lessthanoptimal]

@szarnyasg Thanks for linking to that discussion!

Thanks to @FlorentinD you can remove zero elements too using CommonOps_DSCC.select()

[FlorentinD]

Greetings to Lund @Lundez

Personally, for a graph context, I favor having the possibility of 0 weights.

Maybe we could provide another method that does not set zero elements, instead of changing existing behavior?

[Lundez]

Greetings to Lund @Lundez

Personally, for a graph context, I favor having the possibility of 0 weights.

Maybe we could provide another method that does not set zero elements, instead of changing existing behavior?

Could make sense, or doing it the other way around.
@lessthanoptimal any preference? If you still think this idea makes sense that is

Thanks for everyones input, as soon as I know which direction I should take I'll fix the comments 👍

[ennerf]

I'll chime in here as well. I've tried this optimization a few times, but for my use cases the extra branching always ended up costing more than I got from any subsequent benefits. There may be cases where it's worth it, but I'd vote against making it the default behavior.

[lessthanoptimal]

@ennerf thanks for that insight! What matrix size are you dealing with again? Curious if this could be replicated in a benchmark.

[lessthanoptimal]

@Lundez Right now I'm leaning towards having a distinctive setter that filters out zeros. That will also have a lower bar for being added as it can't potentially break a ton of code. Also forgot to mention in the PR, unit tests are missing.

Unrelated to this PR are you a contributor to kmath? I might have some performance suggestions that could be discussed outside.

[lessthanoptimal]

For what it's worth, it looks like Matlab and Octave filter out and remove 0 when assigning.

a=sparse(10,5)
a(3,4) = 10
a(4,5) = 0
a(3,4) = 0

Octave Outputs:

Compressed Column Sparse (rows = 10, cols = 5, nnz = 0 [0%])

Compressed Column Sparse (rows = 10, cols = 5, nnz = 1 [2%])
(3, 4) ->  10

Compressed Column Sparse (rows = 10, cols = 8, nnz = 1 [1.2%])
(3, 4) ->  10

Compressed Column Sparse (rows = 10, cols = 8, nnz = 0 [0%])

[Lundez]

@lessthanoptimal makes sense 👍

I'm not a contributor, but I guess I could get a PR up.
Currently I've added my own methods on top of their lib to optimize the usage of sparse matrices (such as defining size & only non-zero elements).
The thing I like about kmath is that it'll simplify a transition from pure JVM libraries to native if I need to make that change later for my NLP-library which will include Deep Learning at some time where I need the GPU optimizations (like they got support for ND4J & PyTorch Tensors already).

[ennerf]

@lessthanoptimal I think the largest CSC I used to benchmark in the polynomial spline trajectory project was 9k squared with 15k entries (1k segments), but I assume that the ratio of zeros probably matters more than the raw size. As far as I remember there were very few actual zeros, but unfortunately I'm not using CSC in that project anymore and can't easily reproduce the benchmark data.

For a filtering setter it may be useful to have a minimum threshold for values that can be considered effectively zero, e.g., if( abs(value) < threshold) ignore.

[FlorentinD]

For more intuitive filtering based on the threshold idea by @ennerf , we could provide a 'filter(matrix, threshold)' method which delegates to select with a partly fixed lambda ('x-> |x| > threshold')