Remerge gh-9619: "FIX: Sparse matrix addition/subtraction eliminates explicit zeros"

[ananyashreyjain]

re-merge #9619 .
Binary operations on Sparse matrices removes the explicit zeros. These changes preserve the explicit zeros in the output matrix.

[ananyashreyjain]

Simple check to find if the binary operation is plus or minus.

[perimosocordiae]

I'd much prefer defining a separate function for addition/subtraction, rather than special casing logic here.

[ananyashreyjain]

Even I was doubtful about this but I wanted to keep the code small that's why I choose this special casing logic here. Anyways I have made three separate functions now . One will handle relational operations and other two will handle the arithmetic operations .

[ananyashreyjain]

op(0, Bx[B_pos]) for Bx but just Ax[A_pos] for Ax because if binary operation is minus Bx has to be multiplied with negative sign but this is not the case with Ax .

[ananyashreyjain]

Benchmark results:

before	after	ratio
1.84±0.01ms	2.02±0.01ms	1.10	sparse.Arithmetic.time_arithmetic('csr', 'AA', 'sub')
33.2±0.4 ms	35.3±0.07ms	1.06	sparse.Arithmetic.time_arithmetic('csr', 'BB', 'mul')

[ananyashreyjain]

@perimosocordiae I have separated the part of code performing operations of addition and subtraction from the rest so that other operations remain intact from this change. This separation was necessary because adding an extra condition for checking explicit zeros at bottleneck was affecting the performance adversely . In case of addition and subtraction some checks become redundant and can be replaced with the checks for explicit zeros .

[perimosocordiae]

What's the performance hit for doing the more generic explicit zeros check, instead of special-casing the addition/subtraction case?

[perimosocordiae]

This is the CSR matrix test class, so these should be CSR matrices.

[ananyashreyjain]

You are right, I will change them to CSR matrices.

[perimosocordiae]

I'd much prefer defining a separate function for addition/subtraction, rather than special casing logic here.

[perimosocordiae]

I think you meant "explicit" here.

[ananyashreyjain]

Cx initially contained only the non-zero values but after these changes it can also have the explicit zero values. That means Cx will have the explicit zeros but not the implicit ones.

[ananyashreyjain]

@perimosocordiae for the case of addition and subtraction if the result of operation is zero and the value in one of the matrices is zero then value in the other one will be definitely zero but this doesn't hold good for multiplication and division. In latter case entries in both the matrices will have to be checked which will add one more condition at the bottleneck. This may increase the time to a factor of 1.20 .

[ananyashreyjain]

Benchmark Results:

before	after	ratio
33.6±0.1ms	36.0±0.2ms	1.07	sparse.Arithmetic.time_arithmetic('csr', 'BB', 'mul')
4.86±0.06ms	4.59±0.05ms	0.94	sparse.Arithmetic.time_arithmetic('csr', 'AB', 'multiply')

[ananyashreyjain]

@perimosocordiae I have divided the csr_binop_csr_canonical() and csr_binop_csr_general() into three parts for handling the operations of relation (>, <, <=, >=, etc), addition/subtraction and multiplication/division separately. I have modified these functions a bit so that there is not much performance hit for checking explicit zeros in matrices. After these changes explicit zero check will work for all the arithmetic operations .

[ananyashreyjain]

@perimosocordiae did you get time to go through the changes I made ?

[carldlaird]

Is there an update on this PR?