Skip to content

Conversation

@jessicapriebe
Copy link
Contributor

This patch expands test coverage for all supported sparse matrix block representations, with minor code extensions and fixes.

Copy link
Contributor

@Baunsgaard Baunsgaard left a comment

Choose a reason for hiding this comment

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

LGTM good job, as I said in the comments, I am a bit horrified about some of the bugs found (probably left by me at some point)

But there are nevertheless some cleanups that could be done while you are at it.


//2. correct array lengths
if(_size != nnz && _cindexes.length < nnz && _rindexes.length < nnz && _values.length < nnz) {
if(_size != nnz || _cindexes.length < nnz || _rindexes.length < nnz || _values.length < nnz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is duplicated across the sparse blocks, maybe some of these validity checks can be unified?

for( int i=0; i<rlen; i++ ) {
int apos = pos(i);
int alen = size(i);
if(nnz != alen) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not completely guaranteed if the underlying block is not compacted.

Additionally, while I really like the optimization to leverage the nnz in the comparing vector, the comparing vector also should have the nonzeros at the correct indexes. Perhaps an implementation leveraging materializing a temporary int[] with such offsets, would be faster?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit horrified about how many bugs you found in this file :)


//2. correct array lengths
if(_size != nnz && _ptr.length < clen + 1 && _values.length < nnz && _indexes.length < nnz) {
if(_size != nnz || _ptr.length < clen + 1 || _values.length < nnz || _indexes.length < nnz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is what i mentioned as duplicated, maybe reuse.

//6. a capacity that is no larger than nnz times resize factor.
int capacity = _values.length;
if(capacity > nnz * RESIZE_FACTOR1) {
if(capacity > INIT_CAPACITY && capacity > nnz * RESIZE_FACTOR1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The additional check seems redundant. Maybe I am missing something.

I seem to remember misusing the INIT_CAPACITY somewhere else, and explicitly setting it high to avoid reallocation on appends to MCSR.


//2. correct array lengths
if(_size != nnz && _ptr.length < rlen+1 && _values.length < nnz && _indexes.length < nnz ) {
if( _size != nnz || _ptr.length < rlen+1 || _values.length < nnz || _indexes.length < nnz ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, try the reuse.

}

public static SparseBlock.Type getSparseBlockType(SparseBlock sblock) {
return (sblock instanceof SparseBlockMCSR) ? SparseBlock.Type.MCSR :
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps instead, add a method all of the SparseBlocks need to implement, and have them return their type. This avoids this unmaintainable dependency.

@codecov
Copy link

codecov bot commented Jan 21, 2026

Codecov Report

❌ Patch coverage is 77.71084% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.79%. Comparing base (b394e32) to head (4aa4802).

Files with missing lines Patch % Lines
...org/apache/sysds/runtime/data/SparseBlockMCSC.java 61.90% 9 Missing and 7 partials ⚠️
...org/apache/sysds/runtime/data/SparseBlockMCSR.java 66.66% 3 Missing and 3 partials ⚠️
.../org/apache/sysds/runtime/data/SparseBlockCSC.java 82.75% 3 Missing and 2 partials ⚠️
...ava/org/apache/sysds/runtime/data/SparseBlock.java 80.00% 2 Missing and 2 partials ⚠️
...org/apache/sysds/runtime/data/SparseBlockDCSR.java 89.28% 1 Missing and 2 partials ⚠️
.../org/apache/sysds/runtime/data/SparseBlockCOO.java 93.75% 0 Missing and 1 partial ⚠️
.../org/apache/sysds/runtime/data/SparseBlockCSR.java 80.00% 0 Missing and 1 partial ⚠️
.../apache/sysds/runtime/data/SparseBlockFactory.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2406      +/-   ##
============================================
+ Coverage     71.51%   71.79%   +0.28%     
- Complexity    47441    47654     +213     
============================================
  Files          1539     1539              
  Lines        182605   182669      +64     
  Branches      35916    35941      +25     
============================================
+ Hits         130585   131153     +568     
+ Misses        42028    41523     -505     
- Partials       9992     9993       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants