ValeevGroup / SeQuant

SeQuant: Symbolic Algebra of Tensors over Operators and Scalars
GNU General Public License v3.0
13 stars 4 forks source link

Potentially wrong symmetry deduction for intermediates in eval nodes #178

Open Krzmbrzl opened 5 months ago

Krzmbrzl commented 5 months ago

For tensor contractions, the intermediate tensor that is constructed gets deduced symmetries via https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L379-L382 I believe that this can lead to incorrect results.

Assumption that outer products produce antisymmetric tensors

Inside tensor_symmetry_prod we find https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L310-L321 which checks whether the product of the two given tensors is an outer product (no common indices between the tensors -> no contractions involved). If so, the function returns that the symmetry of the product is antisymmetric.

To my understanding this is wrong for two reasons:

  1. The function doesn't take the input tensor's symmetries into account. If those don't show specific symmetries, their outer product won't either.
  2. Assuming that both input tensors are antisymmetric, the intermediate still can't be labelled as being antisymmetric due to the way symmetry is currently encoded in SeQuant: tensors can only be (anti)symmetric w.r.t. all indices in its bra or ket. However, in case of an outer product, the result contains indices in its bra (same for ket) that belong to the first factor and ones that belong to the second factor. Permutation of indices of different tensors will in general not yield a minus sign. Even worse, I think that in the general case this doesn't lead to any symmetry at all.

Symmetry of fully contracted expressions

In the same function we also find https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L325-L339 that assigns a symmetry to the result of a full contraction. While technically never wrong (regardless of what symmetry is chosen) as there aren't any indices on that intermediate tensor, I am wondering why the effort is taken at all? Shouldn't a full contraction simply yield to a Variable instead of a Tensor as a tensor without indices is effectively a variable, no?

P.S.: That's already done via https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L374-L376 so maybe the code snippet linked above is superfluous?

Braket symmetry

https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L381 This is rather surprising to me. Isn't bra-ket symmetry rather exceptional and essentially only appears for the two-electron integrals? Under what circumstances can we declare the result of any arbitrary tensor product as bra-ket symmetric? :thinking:

bimalgaudel commented 5 months ago

Indeed there might be some issues here: this code was written a while back and we did not actually exploit symmetry of the intermediates during evaluation, so incorrect deductions might have gone unnoticed. The symmetry deduction rules are taken from this paper, Table 1. I invite you to take a look at it since you are already in the loop of symmetry deduction.

Regarding the whole_bk_contracted variable, it could have been better named whole_bra_or_ket_contracted. This case does not imply tensor-times-tensor-to-scalar kind of expression, it is still tensor-times-tensor-to-tensor.

Krzmbrzl commented 5 months ago

Thanks for the link to the paper - haven't seen that one before. I cross-referenced the rules given in table 1 of that paper with the implementation that I see in eval_expr.cpp and this is what I found:

https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L310-L321 This seems to be searching for an outer product of a tensor with itself - with exactly the same indices (contrary to what I wrote in my initial post). I take it that this is essentially a special case of rule 2.5 of the paper. If my interpretation of what the hash-check does is correct, then the deduced antisymmetry should be correct. However, that would also mean that every index appears twice on the intermediate tensor, which seems odd (not more odd than having an outer product of a tensor with itself though).

Should the hash-check allow for different indices on the same tensor, then this would be exactly rule 2.5, in which case the deduced antisymmetry would be wrong. We would only create column-symmetry (particle symmetry) in the intermediate that way, but (in the general case) only between indices originating from different tensors. The permutational symmetry of indices coming from the same tensor would entirely depend on the symmetry properties of that tensor.

https://github.com/ValeevGroup/SeQuant/blob/42558e9f7ca66f76145427d4c8a1bf06715c3029/SeQuant/core/eval_expr.cpp#L333-L339 This seems to implement rule 2.2 and generalize that to two fully antisymmetric tensors, which seems correct :thinking:


If someone could double-check my interpretation of the outer-produce special case handling, I think the deduction is probably fine after all :eyes:

bimalgaudel commented 5 months ago

The outer product of a tensor with itself is symmetric as given in the referenced table (rule 2.5). Could it be antisymmetric for when the tensor itself is antisymmetric (in terms of bra indices and ket indices considered independently? The answer is no -- as far as I could generalize. So the bug fix should be to return the tensor symmetry as Symmetry::symm for such outer products.