pymc-devs / pytensor

PyTensor allows you to define, optimize, and efficiently evaluate mathematical expressions involving multi-dimensional arrays.
https://pytensor.readthedocs.io
Other
366 stars 109 forks source link

Consider not providing sparse `structured_exp` and `structure_log`? #1058

Open ricardoV94 opened 3 weeks ago

ricardoV94 commented 3 weeks ago

Description

from scipy.sparse import random
import pytensor.sparse as ps

x = ps.csc_dmatrix()
x_test = random(5, 5, density=0.5, random_state=1)
y = ps.structured_exp(x)
np.testing.assert_allclose(
    y.eval({x: x_test}).todense(),
    np.exp(x_test.todense()),
)    
Mismatched elements: 13 / 25 (52%)
Max absolute difference: 1.
Max relative difference: 1.
 # x: matrix([[0.      , 0.      , 2.55775 , 0.      , 1.678923],
 #        [0.      , 2.375054, 0.      , 0.      , 1.538332],
 #        [1.097243, 0.      , 0.      , 2.494185, 0.      ],...
 # y: matrix([[1.      , 1.      , 2.55775 , 1.      , 1.678923],
 #        [1.      , 2.375054, 1.      , 1.      , 1.538332],
 #        [1.097243, 1.      , 1.      , 2.494185, 1.      ],...

Since exp(0) != 0, the output is no longer sparse (where missing values are by definition zero). So the wrapper logic where we apply the elemwise operation to the non-sparse entries does not make sense.

Same for log and potentially other Ops

ricardoV94 commented 3 weeks ago

Okay there seems to be a conscious decision in the name: It's called structured_exp instead of just exp. Whereas for methods that are truly sparse like sin, the sparse version is just called sin.

Not sure we should make them available though? They're not used anywhere? If a user is savy enough to know they want a structured computation ignoring the non-zero outputs, they can create one themselves with the structured_monoid decorator?

jessegrabowski commented 2 weeks ago

I agree these are probably too niche to be stand-alone functions. I dislike the name structured_monoid for the helper though. sparse_elemwise seems better?

ricardoV94 commented 2 weeks ago

Doesn't sound as fancy (no objections) :D