Change resizing logic in GMRES

[MichelKern]

I believe the test for deciding when to resize in GMRES in wrong (lines 1281 .... in darcy_vtdd.cc) As written, the test is activated at the first two iterations (which is OK, but wasteful, and not the intended behavior). If the max number of iterations is chosen too small, the code will eventually crash at the last iteration, probably when doing

    Q_side[side][k_counter+1]=q[side];

at line 1445 (not sure).

There are actually two different issues here:

• the variable temp_array_size is declared as an int, but then the comparison temp_array_size<k_counter-2 is between an int and an unsigned int and may be wrong, so temp_array_size should be an unsigned int;
• furthermore, I believe the test should be temp_array_size<k_counter + 2 as otherwise the same line will crash (can you check this ?)

I wrote a small test program to reproduce the behavior. You can compile it as is, or with -D MODIFIED to see the fix I propose. You can see the results in the 2 enclosed files out-mod.txt out-orig.txt

#include <iostream>
using namespace std;

int main(int argc, char *argv[])
{
    unsigned int maxiter = 27;
#ifndef MODIFIED
    int temp_array_size = maxiter/4;
#else
    int temp_array_size = maxiter/4;
#endif    

    unsigned int k_counter = 0;
    std::cout << "Initial size " << temp_array_size << std::endl;
    while (k_counter < maxiter)
      {
          std::cout << "Doing iteration " << k_counter << std::endl;
#ifndef MODIFIED
          if(temp_array_size<k_counter-2){
#else
          if(temp_array_size < k_counter + 2){
#endif
          std::cout << "\nResizing " << std::endl; 
          temp_array_size*=2;
          std::cout << "New size = " << temp_array_size << std::endl;
          }
          k_counter++;
      }
    std::cout << "\nDone" << std::endl;

      return 0;
}

[mjayadharan]

Thanks for pointing it out. I will have a look and try to implement the changes you propose.

On Tue, Sep 8, 2020, 7:56 AM Michel Kern notifications@github.com wrote:

I believe the test for deciding when to resize in GMRES in wrong (lines 1281 .... in darcy_vtdd.cc) As written, the test is activated at the first two iterations (which is OK, but wasteful, and not the intended behavior). If the max number of iterations is chosen too small, the code will eventually crash at the last iteration, probably when doing

Q_side[side][k_counter+1]=q[side];

at line 1445 (not sure).

There are actually two different issues here:

• the variable temp_array_size is declared as an int, but then the comparison temp_array_size<k_counter-2 is between an int and an unsigned int and may be wrong, so temp_array_size should be an unsigned int;
• furthermore, I believe the test should be temp_array_size<k_counter
• 2 as otherwise the same line will crash (can you check this ?)

I wrote a small test program to reproduce the behavior. You can compile it as is, or with -D MODIFIED to see the fix I propose. You can see the results in the 2 enclosed files out-mod.txt https://github.com/mjayadharan/MMMFE-ST-DD/files/5188090/out-mod.txt out-orig.txt https://github.com/mjayadharan/MMMFE-ST-DD/files/5188091/out-orig.txt

include

using namespace std;

int main(int argc, char *argv[]) { unsigned int maxiter = 27;

ifndef MODIFIED

int temp_array_size = maxiter/4;

else

int temp_array_size = maxiter/4;

endif

unsigned int k_counter = 0;
std::cout << "Initial size " << temp_array_size << std::endl;
while (k_counter < maxiter)
{
    std::cout << "Doing iteration " << k_counter << std::endl;

ifndef MODIFIED

    if(temp_array_size<k_counter-2){

else

    if(temp_array_size < k_counter + 2){

endif

    std::cout << "\nResizing " << std::endl;
    temp_array_size*=2;
    std::cout << "New size = " << temp_array_size << std::endl;
    }
    k_counter++;
}
std::cout << "\nDone" << std::endl;

return 0;

}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mjayadharan/MMMFE-ST-DD/issues/28, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIR5RWMTC53W7SAGYZDTWV3SEYLYHANCNFSM4Q736BEA .

[mjayadharan]

Figured out the problem. 1) You are right regarding needing to put k_counter +2 instead of k_counter -2. This was a bug which was not really noticed before. 2) The reason why the refinement is done in the first two iterations is because, since k_counter is an unsigned int starting from 0, for the first two iterations the value of it is <2 and hence if I do k_counter-2, I should get a negative int, which is not represented by unsigned int. What I get indeed is a really big positive integer, which in turns satisfy the condition temp_array_size <k_counter-2. Modifying the code to check the condition with k_counter +2 instead of k_counter-1, should fix this issue as well, since we never get a negative int with this new condition.

Thanks for the comments and the test case, which really made the debugging look easy.

[MichelKern]

Thanks, this is a bug that does not really appear "in real life", unless you try to do something silly :)