Jutho / TensorOperations.jl

Julia package for tensor contractions and related operations
https://jutho.github.io/TensorOperations.jl/stable/
Other
443 stars 56 forks source link

Cryptic `IndexError` messages #127

Closed fatteneder closed 1 year ago

fatteneder commented 1 year ago

Hi,

firstly, thanks for the great package.

MWE

# mwe.jl
using TensorOperations
A = randn(3,3)
@tensor B[i,j] := A[u,j]

yields

julia> include("mwe.jl")
ERROR: LoadError: TensorOperations.IndexError{String}("non-matching indices between left and right hand side: var\"##315\"[i, j] := var\"##314\"[u, j]")
...

Problem seems because of the gensym()s being inserted before the expression is tensorifyed. Moving extractensorobjects from preprocessors to postprocessors in the internal constructor TensorParser would fix that, e.g.

julia> include("mwe.jl")
ERROR: LoadError: TensorOperations.IndexError{String}("non-matching indices between left and right hand side: B[i, j] := A[u, j]")
...

Would you be interested in a PR with the above fix? Or is that going to interfere with something else?

Jutho commented 1 year ago

This situation is not ideal. The reason for substituting variable names in the macro is that sometimes, the tensor object is not a single variable but some expression, e.g., you could do

@tensor B[i,j] := (view(some_dict["some_key"], :, 1:10, :))[u,j]

Since the macro rewrites this expression in a way that uses the objects several times, I need to store these expressions in a variable to prevent the expression from being evaluated multiple times. What could probably help is that, in those cases where this expression is simply a variable, to either not use the new name, or generate a new name that contains the original variable within, something like:

julia> gensym(:A)
Symbol("##A#313")
fatteneder commented 1 year ago

Oh, I see what you mean.

Having Symbol("##A#313") would be an improvement, but still not optimal IMO.

Another way to do it could be to delay the (full) replacement of the expressions with gensyms till after they have been checked inside tensorify. In other words, 1) extracttensorobjects should replace an obj expression with an intermediate (gensym(obj),obj) tuple, 2) later in tensorify one could then access the initial obj for error messages and discard it if checks passed.

I did not try this method yet, but might do soon, so I am not 100% sure it would work. What do you think?