adrhill / SparseConnectivityTracer.jl

Fast operator-overloading Jacobian & Hessian sparsity detection.
MIT License
27 stars 2 forks source link

Increase sparsity for `Diagonal` inputs #165

Closed adrhill closed 3 months ago

adrhill commented 3 months ago

By having trace_input create a Diagonal matrix of tracers with only the indices from diagind, the sparsity of patterns can be increased by a lot on Diagonal inputs:

image

Previously, this would have returned a dense pattern of ones (see #147).

codecov-commenter commented 3 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 87.24490% with 25 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (199899d) to head (cec44b8). Report is 1 commits behind head on main.

Files Patch % Lines
test/test_arrays.jl 86.18% 25 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #165 +/- ## ========================================== - Coverage 92.70% 91.83% -0.88% ========================================== Files 37 37 Lines 1590 1763 +173 ========================================== + Hits 1474 1619 +145 - Misses 116 144 +28 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gdalle commented 3 months ago

Is this even necessary? I understand the need to handle Diagonal matrices created inside the function we trace. But I strongly doubt that there is a practical use case in which people would pass a Diagonal as input of sparsity detection. If you know that you only care about a Diagonal, you will pass the corresponding vector of coefficients as input and build the Diagonal internally.

adrhill commented 3 months ago

This issue started with #145, where a user had a ComponentArray input. Until then, every input was converted to a dense Matrix.

The functionality of this PR is just 10 LOC to obtain much sparser patterns. Most of it is rewriting our array tests to be more flexible.

gdalle commented 3 months ago

I think this PR does two things: 1) more overloads for Diagonal inside generic code and 2) handling a Diagonal as input of sparsity detection. I'm only questioning the legitimacy of 2. While ComponentArrays or Symmetric matrices as input to sparsity detection make sense, Diagonal matrices make less sense to me.

adrhill commented 3 months ago

Point 2) is 4 LOC and worth it for easier testing of point 1), even if no user ever has Diagonal inputs.

gdalle commented 3 months ago

I'm just asking where you draw the line. To push this reasoning to the extreme, we want code to work if it uses FillArrays internally, but no sane person would give a FillArray as input of sparsity detection

adrhill commented 3 months ago

I'm just asking where you draw the line.

I definitely want to support SparseArray inputs in the future.

As soon as we "internally" support an array type, it makes sense to write a matching trace_input method. It makes testing and debugging that much easier.

To push this reasoning to the extreme, we want code to work if it uses FillArrays internally, but no sane person would give a FillArray as input of sparsity detection

Your point reminds me that we don't explicitly test FillArrays yet.