TRIQS / triqs

a Toolbox for Research on Interacting Quantum Systems
https://triqs.github.io
GNU General Public License v3.0
141 stars 72 forks source link

Refactored higher-rank updates trigger assertion #868

Closed hmenke closed 1 year ago

hmenke commented 1 year ago

Prerequisites

Description

Higher-rank updates was merged after some extensive refactoring which seems to have broken the feature.

Steps to Reproduce

Consider the Dimer benchmark with these additional changes:

--- a/Dimer/model.py
+++ b/Dimer/model.py
@@ -44,7 +44,7 @@ c_vec =     { s: matrix([[c(s,o)] for o in range(n_orb)]) for s in block_names }
 h_0 = sum(c_dag_vec[s] * h_0_mat * c_vec[s] for s in block_names)[0,0]

 Umat, Upmat = U_matrix_kanamori(n_orb, U_int=U, J_hund=J)
-h_int = h_int_kanamori(block_names, range(n_orb), Umat, Upmat, J, off_diag=True)
+h_int = h_int_kanamori(block_names, n_orb, Umat, Upmat, J, off_diag=True)

 h_imp = h_0 + h_int

diff --git a/common/ctint b/common/ctint
index dfa9c1f..5df9d1d 100755
--- a/common/ctint
+++ b/common/ctint
@@ -26,7 +26,8 @@ solve_params = {
         'h_int' : h_int,
         'n_warmup_cycles' : 10000,
         'n_cycles' : 1000000,
-        'length_cycle' : 100
+        'length_cycle' : 100,
+        'use_double_insertion': True,
         }

 start = time.time()

This crashes CT-Int with

mc_generic: Exception occurs on node 0
Triqs runtime error
    at /home/og85ixak/Code/triqs/triqs_unstable/install/include/triqs/det_manip/det_manip.hpp : 830

i[l] != i[l + 1] and 0 <= i[l] and i[l] < N
Exception was thrown on node

Expected behavior: Script completes normally.

Actual behavior: Assertion is triggered.

Versions

$ python3 -c 'import triqs_ctint.version; triqs_ctint.version.show_git_hash()'
Warning: could not identify MPI environment!
Starting serial run at: 2023-01-16 14:30:53.448205

You are using triqs_ctint git hash bb0a5a543ec7812c670149a1b93feeff743ecc0a based on triqs git hash edc1668db666a58b1c7e6a0b21fc2407c4b853ed
hmenke commented 1 year ago

Error No. 1 This is a self-assignment to the parameter and will not update the member variable k.

https://github.com/TRIQS/triqs/blob/edc1668db666a58b1c7e6a0b21fc2407c4b853ed/c%2B%2B/triqs/det_manip/det_manip.hpp#L147-L148

Proposed solution: Rename the parameter to k_ and do k = k_; or use this->k = k;. This would have been caught immediately if the parameter was made constant (long const k).

Error No. 2 This will never shrink wk, but this means that wk.ksi will never shrink. This leads to size confusion when a triple insertion is followed by a double insertion.

https://github.com/TRIQS/triqs/blob/edc1668db666a58b1c7e6a0b21fc2407c4b853ed/c%2B%2B/triqs/det_manip/det_manip.hpp#L233-L237

Proposed solution: Restore the old code maybe?

Wentzell commented 1 year ago

@hmenke Thank you for pointing out this issue with the merge.

Regarding the second point: It shouldn't be a problem that wk never shrinks? We access into the underlying matrices always through proper indexing or sliced views, or am I wrong? Not shrinking wk when jumping between say triple and double inserts avoids unnecessary reallocations. kmax was introduced for that reason, similar to Nmax.

hmenke commented 1 year ago

The else-branch in wk.det_ksi() and pretty much all accesses to wk.ksi are not performed by range.

https://github.com/TRIQS/triqs/blob/edc1668db666a58b1c7e6a0b21fc2407c4b853ed/c++/triqs/det_manip/det_manip.hpp#L173-L175

Many more like the following:

https://github.com/TRIQS/triqs/blob/edc1668db666a58b1c7e6a0b21fc2407c4b853ed/c++/triqs/det_manip/det_manip.hpp#L669

https://github.com/TRIQS/triqs/blob/edc1668db666a58b1c7e6a0b21fc2407c4b853ed/c++/triqs/det_manip/det_manip.hpp#L724

Wentzell commented 1 year ago

I have addressed the above points in https://github.com/TRIQS/triqs/commit/232cbe0be8a4418567411474931c1e40a06fb819 Could you please have a look try that with your example? Also, would it be possible that you extend the tests of your initial PR to cover the failures you mention?

Wentzell commented 1 year ago

All related issues are addressed in the current unstable branch