Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potentially wrong symmetry deduction for intermediates in eval nodes #178

Open
Krzmbrzl opened this issue Feb 6, 2024 · 3 comments
Open

Comments

@Krzmbrzl
Copy link
Collaborator

Krzmbrzl commented Feb 6, 2024

For tensor contractions, the intermediate tensor that is constructed gets deduced symmetries via

auto ts = tensor_symmetry_prod(left, right);
auto ps = particle_symmetry(ts);
auto bks = get_default_context().braket_symmetry();
return ex<Tensor>(L"I", bra, ket, ts, bks, ps);

I believe that this can lead to incorrect results.

Assumption that outer products produce antisymmetric tensors

Inside tensor_symmetry_prod we find

if (hash::value(left) == hash::value(right)) {
// potential outer product of the same tensor
auto const uniq_idxs =
ranges::views::concat(tnsr1.const_braket(), tnsr2.const_braket()) |
ranges::to<index_set_t>;
if (static_cast<std::size_t>(ranges::distance(uniq_idxs)) ==
tnsr1.const_braket().size() + tnsr2.const_braket().size()) {
// outer product confirmed
return Symmetry::antisymm;
}
}

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

bool whole_bk_contracted = (all_common_indices(tnsr1.bra(), tnsr2.ket()) ||
all_common_indices(tnsr1.ket(), tnsr2.bra()));
auto sym1 = tnsr1.symmetry();
auto sym2 = tnsr2.symmetry();
assert(sym1 != Symmetry::invalid);
assert(sym2 != Symmetry::invalid);
if (whole_bk_contracted &&
!(sym1 == Symmetry::nonsymm || sym2 == Symmetry::nonsymm)) {
imed_sym = sym1 == sym2 ? sym1 : Symmetry::symm;
} else {
imed_sym = Symmetry::nonsymm;
}

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

if (bra.empty() && ket.empty()) {
// dot product
return ex<Variable>(var_label);

so maybe the code snippet linked above is superfluous?

Braket symmetry

auto bks = get_default_context().braket_symmetry();

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? 🤔

@bimalgaudel
Copy link
Member

bimalgaudel commented Feb 6, 2024

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
Copy link
Collaborator Author

Krzmbrzl commented Feb 6, 2024

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:

if (hash::value(left) == hash::value(right)) {
// potential outer product of the same tensor
auto const uniq_idxs =
ranges::views::concat(tnsr1.const_braket(), tnsr2.const_braket()) |
ranges::to<index_set_t>;
if (static_cast<std::size_t>(ranges::distance(uniq_idxs)) ==
tnsr1.const_braket().size() + tnsr2.const_braket().size()) {
// outer product confirmed
return Symmetry::antisymm;
}
}

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.

if (whole_bk_contracted &&
!(sym1 == Symmetry::nonsymm || sym2 == Symmetry::nonsymm)) {
imed_sym = sym1 == sym2 ? sym1 : Symmetry::symm;
} else {
imed_sym = Symmetry::nonsymm;
}

This seems to implement rule 2.2 and generalize that to two fully antisymmetric tensors, which seems correct 🤔


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

@bimalgaudel
Copy link
Member

bimalgaudel commented Feb 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants