Ising model simulation












3












$begingroup$


I have written this code to simulate Ising Model at one particular temperature in presence of magnetic field to observe hysteresis effect using the metropolis algorithm.



While the code runs and gave me a desired output, it is a badly written code(I feel so) because of my lack of coding experience. Could you help me out as to how could I have written this in a better way? Or what can I do to optimize it next time I write such a code? Or are there any inbuilt functions I could have called instead of writing a block?



I borrowed the random number generator directly from someone's answer to a thread on this site. I cannot find the exact thread, apologies! (I will cite it once I do).



Function to assign random spins to the lattice



int spin(int r) 
{
int s;
if(r>5)
{
s=+1;
}
else
{
s=-1;
}

return s;
}


Random number generator



float prandom(int i,int N)
{
std::random_device rd; //Will be used to obtain a seed for the random number engine
std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
std::uniform_real_distribution<> dis(i,N);
// Use dis to transform the random unsigned int generated by gen into a
// double in [1, 2). Each call to dis(gen) generates a new random double
int t = dis(gen);
return t;
}


Function to randomly flip the spins



I am selecting a random site and seeing how the total energy will change by calculating the energy dE for the neighbouring sites. If the energy is negative then I make the spin flip permanent if it is not negative then I gave a probability exp(-dE) by which it can flip the spin



std::vector< std::vector < int > > flip (int N,std::vector< std::vector < int > >lattice, float beta,int tab,float H)
{
int a =prandom(0,N);
int b =prandom(0,N);
int s=lattice[a][b];
float dE=(2*s*H)+(2*s*(lattice[tab[a+2]][b]+lattice[tab[a]][b]+lattice[a][tab[b+2]]+lattice[a][tab[b]]));
//std::cout<<dE<<"t"<<a<<"t"<<b<<"n";
if(dE<0)
{
s=-1*s;
}
else
{
float k = 1.0*prandom(0.0,1000)/1000;
float H = exp(-beta*dE);
if(k<=H)
{
s=-1*s;
}
else
{
s = 1*s;
}
}
lattice[a][b]=s;
return lattice;
}


Main program



int main()
{
std::ofstream outdata;
outdata.open("ising_model_field_final2.txt");
int a,b,N=20,i,j,k,r,t,sweep=1500;
float M=0,M_sweep=0,H=-0.10;
int tab[N];
tab[0] = N-1;
tab[N+1] = 0;
for (i=1;i<=N;i++)
{
tab[i]=i-1; // this is the periodic boundary condition to make my lattice infinite (lattice site [x][0] is a neighbour of [x][N] and so on..)
}
float T, beta;
//beta=1.0/T; // boltzman constant is assumed to be 1.
//creating a 2d lattice and populating it
std::vector< std::vector < int > >lattice;

//populate the lattice
for (i=0; i<N; i++)
{
std::vector< int > row; //create a row of the lattice
for (j=0;j<N;j++)
{
row.push_back(-1); //populate the row vector
}
lattice.push_back(row); //populate the column vector
}
lattice=flip(N,lattice,beta, tab,H);
/* for(i=0;i<N;i++)
{
for(j=0;j<N;j++)
{
std::cout<<lattice[j][i]<<"t";
}
std::cout<<std::endl;
}*/

for(int temp=1;temp<=30;temp++)
{
if(temp>15)
{
H=H-0.015;
}
else
{
H=H+0.015;
}
//M=0;
T=2.2;
beta=1.0/T;


std::cout<<beta<<"n";
for(i=0;i<=sweep;i++)
{
//T=0.1*i;


//printf("Sweep = %dn",i);
for(j=1;j<=N*N;j++)
{
lattice=flip(N,lattice,beta, tab,H);
}
M_sweep=0;
for(t=0;t<N;t++)
{
for(int u=0;u<N;u++)
{
if(i>=500)
{M_sweep=M_sweep+lattice[t][u];}
}
//std::cout<<"Mag="<<M<<"t";
}
M=M+ M_sweep/(N*N);
//std::cout<<"Mag="<<M<<"t";
}
M=M/(sweep-1000);
std::cout<<T<<"n";
outdata << M <<"t"<< H <<"n";
}


for(i=0;i<N;i++)
{
for(j=0;j<N;j++)
{
std::cout<<lattice[j][i]<<"t";
}
std::cout<<std::endl;
}
outdata.close();
}









share|improve this question









New contributor




aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$endgroup$

















    3












    $begingroup$


    I have written this code to simulate Ising Model at one particular temperature in presence of magnetic field to observe hysteresis effect using the metropolis algorithm.



    While the code runs and gave me a desired output, it is a badly written code(I feel so) because of my lack of coding experience. Could you help me out as to how could I have written this in a better way? Or what can I do to optimize it next time I write such a code? Or are there any inbuilt functions I could have called instead of writing a block?



    I borrowed the random number generator directly from someone's answer to a thread on this site. I cannot find the exact thread, apologies! (I will cite it once I do).



    Function to assign random spins to the lattice



    int spin(int r) 
    {
    int s;
    if(r>5)
    {
    s=+1;
    }
    else
    {
    s=-1;
    }

    return s;
    }


    Random number generator



    float prandom(int i,int N)
    {
    std::random_device rd; //Will be used to obtain a seed for the random number engine
    std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
    std::uniform_real_distribution<> dis(i,N);
    // Use dis to transform the random unsigned int generated by gen into a
    // double in [1, 2). Each call to dis(gen) generates a new random double
    int t = dis(gen);
    return t;
    }


    Function to randomly flip the spins



    I am selecting a random site and seeing how the total energy will change by calculating the energy dE for the neighbouring sites. If the energy is negative then I make the spin flip permanent if it is not negative then I gave a probability exp(-dE) by which it can flip the spin



    std::vector< std::vector < int > > flip (int N,std::vector< std::vector < int > >lattice, float beta,int tab,float H)
    {
    int a =prandom(0,N);
    int b =prandom(0,N);
    int s=lattice[a][b];
    float dE=(2*s*H)+(2*s*(lattice[tab[a+2]][b]+lattice[tab[a]][b]+lattice[a][tab[b+2]]+lattice[a][tab[b]]));
    //std::cout<<dE<<"t"<<a<<"t"<<b<<"n";
    if(dE<0)
    {
    s=-1*s;
    }
    else
    {
    float k = 1.0*prandom(0.0,1000)/1000;
    float H = exp(-beta*dE);
    if(k<=H)
    {
    s=-1*s;
    }
    else
    {
    s = 1*s;
    }
    }
    lattice[a][b]=s;
    return lattice;
    }


    Main program



    int main()
    {
    std::ofstream outdata;
    outdata.open("ising_model_field_final2.txt");
    int a,b,N=20,i,j,k,r,t,sweep=1500;
    float M=0,M_sweep=0,H=-0.10;
    int tab[N];
    tab[0] = N-1;
    tab[N+1] = 0;
    for (i=1;i<=N;i++)
    {
    tab[i]=i-1; // this is the periodic boundary condition to make my lattice infinite (lattice site [x][0] is a neighbour of [x][N] and so on..)
    }
    float T, beta;
    //beta=1.0/T; // boltzman constant is assumed to be 1.
    //creating a 2d lattice and populating it
    std::vector< std::vector < int > >lattice;

    //populate the lattice
    for (i=0; i<N; i++)
    {
    std::vector< int > row; //create a row of the lattice
    for (j=0;j<N;j++)
    {
    row.push_back(-1); //populate the row vector
    }
    lattice.push_back(row); //populate the column vector
    }
    lattice=flip(N,lattice,beta, tab,H);
    /* for(i=0;i<N;i++)
    {
    for(j=0;j<N;j++)
    {
    std::cout<<lattice[j][i]<<"t";
    }
    std::cout<<std::endl;
    }*/

    for(int temp=1;temp<=30;temp++)
    {
    if(temp>15)
    {
    H=H-0.015;
    }
    else
    {
    H=H+0.015;
    }
    //M=0;
    T=2.2;
    beta=1.0/T;


    std::cout<<beta<<"n";
    for(i=0;i<=sweep;i++)
    {
    //T=0.1*i;


    //printf("Sweep = %dn",i);
    for(j=1;j<=N*N;j++)
    {
    lattice=flip(N,lattice,beta, tab,H);
    }
    M_sweep=0;
    for(t=0;t<N;t++)
    {
    for(int u=0;u<N;u++)
    {
    if(i>=500)
    {M_sweep=M_sweep+lattice[t][u];}
    }
    //std::cout<<"Mag="<<M<<"t";
    }
    M=M+ M_sweep/(N*N);
    //std::cout<<"Mag="<<M<<"t";
    }
    M=M/(sweep-1000);
    std::cout<<T<<"n";
    outdata << M <<"t"<< H <<"n";
    }


    for(i=0;i<N;i++)
    {
    for(j=0;j<N;j++)
    {
    std::cout<<lattice[j][i]<<"t";
    }
    std::cout<<std::endl;
    }
    outdata.close();
    }









    share|improve this question









    New contributor




    aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
    Check out our Code of Conduct.







    $endgroup$















      3












      3








      3





      $begingroup$


      I have written this code to simulate Ising Model at one particular temperature in presence of magnetic field to observe hysteresis effect using the metropolis algorithm.



      While the code runs and gave me a desired output, it is a badly written code(I feel so) because of my lack of coding experience. Could you help me out as to how could I have written this in a better way? Or what can I do to optimize it next time I write such a code? Or are there any inbuilt functions I could have called instead of writing a block?



      I borrowed the random number generator directly from someone's answer to a thread on this site. I cannot find the exact thread, apologies! (I will cite it once I do).



      Function to assign random spins to the lattice



      int spin(int r) 
      {
      int s;
      if(r>5)
      {
      s=+1;
      }
      else
      {
      s=-1;
      }

      return s;
      }


      Random number generator



      float prandom(int i,int N)
      {
      std::random_device rd; //Will be used to obtain a seed for the random number engine
      std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
      std::uniform_real_distribution<> dis(i,N);
      // Use dis to transform the random unsigned int generated by gen into a
      // double in [1, 2). Each call to dis(gen) generates a new random double
      int t = dis(gen);
      return t;
      }


      Function to randomly flip the spins



      I am selecting a random site and seeing how the total energy will change by calculating the energy dE for the neighbouring sites. If the energy is negative then I make the spin flip permanent if it is not negative then I gave a probability exp(-dE) by which it can flip the spin



      std::vector< std::vector < int > > flip (int N,std::vector< std::vector < int > >lattice, float beta,int tab,float H)
      {
      int a =prandom(0,N);
      int b =prandom(0,N);
      int s=lattice[a][b];
      float dE=(2*s*H)+(2*s*(lattice[tab[a+2]][b]+lattice[tab[a]][b]+lattice[a][tab[b+2]]+lattice[a][tab[b]]));
      //std::cout<<dE<<"t"<<a<<"t"<<b<<"n";
      if(dE<0)
      {
      s=-1*s;
      }
      else
      {
      float k = 1.0*prandom(0.0,1000)/1000;
      float H = exp(-beta*dE);
      if(k<=H)
      {
      s=-1*s;
      }
      else
      {
      s = 1*s;
      }
      }
      lattice[a][b]=s;
      return lattice;
      }


      Main program



      int main()
      {
      std::ofstream outdata;
      outdata.open("ising_model_field_final2.txt");
      int a,b,N=20,i,j,k,r,t,sweep=1500;
      float M=0,M_sweep=0,H=-0.10;
      int tab[N];
      tab[0] = N-1;
      tab[N+1] = 0;
      for (i=1;i<=N;i++)
      {
      tab[i]=i-1; // this is the periodic boundary condition to make my lattice infinite (lattice site [x][0] is a neighbour of [x][N] and so on..)
      }
      float T, beta;
      //beta=1.0/T; // boltzman constant is assumed to be 1.
      //creating a 2d lattice and populating it
      std::vector< std::vector < int > >lattice;

      //populate the lattice
      for (i=0; i<N; i++)
      {
      std::vector< int > row; //create a row of the lattice
      for (j=0;j<N;j++)
      {
      row.push_back(-1); //populate the row vector
      }
      lattice.push_back(row); //populate the column vector
      }
      lattice=flip(N,lattice,beta, tab,H);
      /* for(i=0;i<N;i++)
      {
      for(j=0;j<N;j++)
      {
      std::cout<<lattice[j][i]<<"t";
      }
      std::cout<<std::endl;
      }*/

      for(int temp=1;temp<=30;temp++)
      {
      if(temp>15)
      {
      H=H-0.015;
      }
      else
      {
      H=H+0.015;
      }
      //M=0;
      T=2.2;
      beta=1.0/T;


      std::cout<<beta<<"n";
      for(i=0;i<=sweep;i++)
      {
      //T=0.1*i;


      //printf("Sweep = %dn",i);
      for(j=1;j<=N*N;j++)
      {
      lattice=flip(N,lattice,beta, tab,H);
      }
      M_sweep=0;
      for(t=0;t<N;t++)
      {
      for(int u=0;u<N;u++)
      {
      if(i>=500)
      {M_sweep=M_sweep+lattice[t][u];}
      }
      //std::cout<<"Mag="<<M<<"t";
      }
      M=M+ M_sweep/(N*N);
      //std::cout<<"Mag="<<M<<"t";
      }
      M=M/(sweep-1000);
      std::cout<<T<<"n";
      outdata << M <<"t"<< H <<"n";
      }


      for(i=0;i<N;i++)
      {
      for(j=0;j<N;j++)
      {
      std::cout<<lattice[j][i]<<"t";
      }
      std::cout<<std::endl;
      }
      outdata.close();
      }









      share|improve this question









      New contributor




      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.







      $endgroup$




      I have written this code to simulate Ising Model at one particular temperature in presence of magnetic field to observe hysteresis effect using the metropolis algorithm.



      While the code runs and gave me a desired output, it is a badly written code(I feel so) because of my lack of coding experience. Could you help me out as to how could I have written this in a better way? Or what can I do to optimize it next time I write such a code? Or are there any inbuilt functions I could have called instead of writing a block?



      I borrowed the random number generator directly from someone's answer to a thread on this site. I cannot find the exact thread, apologies! (I will cite it once I do).



      Function to assign random spins to the lattice



      int spin(int r) 
      {
      int s;
      if(r>5)
      {
      s=+1;
      }
      else
      {
      s=-1;
      }

      return s;
      }


      Random number generator



      float prandom(int i,int N)
      {
      std::random_device rd; //Will be used to obtain a seed for the random number engine
      std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
      std::uniform_real_distribution<> dis(i,N);
      // Use dis to transform the random unsigned int generated by gen into a
      // double in [1, 2). Each call to dis(gen) generates a new random double
      int t = dis(gen);
      return t;
      }


      Function to randomly flip the spins



      I am selecting a random site and seeing how the total energy will change by calculating the energy dE for the neighbouring sites. If the energy is negative then I make the spin flip permanent if it is not negative then I gave a probability exp(-dE) by which it can flip the spin



      std::vector< std::vector < int > > flip (int N,std::vector< std::vector < int > >lattice, float beta,int tab,float H)
      {
      int a =prandom(0,N);
      int b =prandom(0,N);
      int s=lattice[a][b];
      float dE=(2*s*H)+(2*s*(lattice[tab[a+2]][b]+lattice[tab[a]][b]+lattice[a][tab[b+2]]+lattice[a][tab[b]]));
      //std::cout<<dE<<"t"<<a<<"t"<<b<<"n";
      if(dE<0)
      {
      s=-1*s;
      }
      else
      {
      float k = 1.0*prandom(0.0,1000)/1000;
      float H = exp(-beta*dE);
      if(k<=H)
      {
      s=-1*s;
      }
      else
      {
      s = 1*s;
      }
      }
      lattice[a][b]=s;
      return lattice;
      }


      Main program



      int main()
      {
      std::ofstream outdata;
      outdata.open("ising_model_field_final2.txt");
      int a,b,N=20,i,j,k,r,t,sweep=1500;
      float M=0,M_sweep=0,H=-0.10;
      int tab[N];
      tab[0] = N-1;
      tab[N+1] = 0;
      for (i=1;i<=N;i++)
      {
      tab[i]=i-1; // this is the periodic boundary condition to make my lattice infinite (lattice site [x][0] is a neighbour of [x][N] and so on..)
      }
      float T, beta;
      //beta=1.0/T; // boltzman constant is assumed to be 1.
      //creating a 2d lattice and populating it
      std::vector< std::vector < int > >lattice;

      //populate the lattice
      for (i=0; i<N; i++)
      {
      std::vector< int > row; //create a row of the lattice
      for (j=0;j<N;j++)
      {
      row.push_back(-1); //populate the row vector
      }
      lattice.push_back(row); //populate the column vector
      }
      lattice=flip(N,lattice,beta, tab,H);
      /* for(i=0;i<N;i++)
      {
      for(j=0;j<N;j++)
      {
      std::cout<<lattice[j][i]<<"t";
      }
      std::cout<<std::endl;
      }*/

      for(int temp=1;temp<=30;temp++)
      {
      if(temp>15)
      {
      H=H-0.015;
      }
      else
      {
      H=H+0.015;
      }
      //M=0;
      T=2.2;
      beta=1.0/T;


      std::cout<<beta<<"n";
      for(i=0;i<=sweep;i++)
      {
      //T=0.1*i;


      //printf("Sweep = %dn",i);
      for(j=1;j<=N*N;j++)
      {
      lattice=flip(N,lattice,beta, tab,H);
      }
      M_sweep=0;
      for(t=0;t<N;t++)
      {
      for(int u=0;u<N;u++)
      {
      if(i>=500)
      {M_sweep=M_sweep+lattice[t][u];}
      }
      //std::cout<<"Mag="<<M<<"t";
      }
      M=M+ M_sweep/(N*N);
      //std::cout<<"Mag="<<M<<"t";
      }
      M=M/(sweep-1000);
      std::cout<<T<<"n";
      outdata << M <<"t"<< H <<"n";
      }


      for(i=0;i<N;i++)
      {
      for(j=0;j<N;j++)
      {
      std::cout<<lattice[j][i]<<"t";
      }
      std::cout<<std::endl;
      }
      outdata.close();
      }






      c++ simulation physics






      share|improve this question









      New contributor




      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.











      share|improve this question









      New contributor




      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      share|improve this question




      share|improve this question








      edited 36 mins ago









      200_success

      131k17156420




      131k17156420






      New contributor




      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.









      asked 6 hours ago









      aargieeaargiee

      161




      161




      New contributor




      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.





      New contributor





      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






      aargiee is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
      Check out our Code of Conduct.






















          2 Answers
          2






          active

          oldest

          votes


















          3












          $begingroup$

          There are many, many things wrong with your code; I'll list some, and then I advise you to fix as many of them as you can (and more) and then repost a new question with the revised code.



          First of all, you should compile your code with -W -Wall -Wextra (or -W4 if you use MSVC). Read the first warning diagnostic. Fix it. Recompile. Read the first diagnostic. Fix it. ...until all the warnings are completely gone.



          Procedural nit: When you post code to CodeReview, please post it in one big cut-and-pasteable chunk, or at least just a couple of chunks. Your current post mixes code and commentary in a way that makes it hard for the reviewer to cut-and-paste the whole piece of code into a file for testing.





          Speaking of commentary:



              float T, beta;
          //beta=1.0/T; // boltzman constant is assumed to be 1.
          //creating a 2d lattice and populating it
          std::vector< std::vector < int > >lattice;


          Is beta supposed to be 1.0 / T, or not? By commenting out that line of code, you make it invisible to the compiler. Better make it invisible to the reader, too, and just delete it! (If you're commenting things out to preserve the "history" of the code, read up on version control systems like git, and then use one. It's easy!)



          Furthermore, since you don't initialize beta, you can probably just get rid of the variable entirely.



          Finally, the idiomatic way to place the whitespace when you're defining a variable of type std::vector<std::vector<int>> is simply thus:



          std::vector<std::vector<int>> lattice;


          Notice the space between the variable's type and its name; and the lack of space anywhere else.





          Populating that lattice can be done quickly and easily using vector's "filling" constructor:



          std::vector<std::vector<int>> lattice(N, std::vector<int>(N, -1));


          Now you don't need those nested for loops anymore!





          Going back up to the top of your code:



          int spin(int r) 
          {
          int s;
          if(r>5)
          {
          s=+1;
          }
          else
          {
          s=-1;
          }

          return s;
          }


          Replace this 14-line function with a 4-line function that does the same thing more clearly and simply:



          int spin(int r)
          {
          return (r > 5) ? 1 : -1;
          }


          No local variables, no mutation, no startling use of the =+ "operator"; and perhaps most importantly for the reader, there's now 10 more lines available on my screen so I can look at other code at the same time! Vertical real estate can be important for reading comprehension.





          float prandom(int i,int N)
          {
          std::random_device rd; //Will be used to obtain a seed for the random number engine
          std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
          std::uniform_real_distribution<> dis(i,N);
          // Use dis to transform the random unsigned int generated by gen into a
          // double in [1, 2). Each call to dis(gen) generates a new random double
          int t = dis(gen);
          return t;
          }


          This is wrong in at least two ways. First, most importantly: you're constructing a std::random_device on each call to this function. That is extremely expensive. Think of std::random_device as an open file handle to /dev/urandom, because that's what it is, under the hood. That means every time you call prandom, you're opening a file, reading some bytes, and closing it again!



          You should keep a global random number generator, initialized just once at the start of the program. One way to do this (not cheap, but not as expensive as opening a file on every call to prandom) would be



          float prandom(int i,int N)
          {
          static std::mt19937 gen = () {
          std::random_device rd;
          return std::mt19937(rd());
          }();
          std::uniform_real_distribution<float> dis(i, N);
          // ...


          Notice that uniform_real_distribution<> is secretly an alias for uniform_real_distribution<double>. Your code doesn't use doubles; it uses floats. So it's (always) better to be explicit and say what type you mean — you have less chance of getting it wrong by accident!



              int t = dis(gen);
          return t;


          ...And then you go ahead and stuff the return value into an int anyway! So what was the point of using a uniform_real_distribution in the first place? And what's the point of returning a float from prandom? Did you mean to simply



          return dis(gen);


          instead?



          You're also seeding your PRNG wrong, but seeding it correctly is a huge headache in C++17, so never mind that.





                  if(temp>15)
          {
          H=H-0.015;
          }


          Please indent your code correctly. You can use the clang-format command-line tool to automatically indent everything, or if you use a graphical IDE it almost certainly has an "Indent" option somewhere in the menus.





          That's enough for one day. As I said above, I advise you to fix as much as possible (that is, fix everything I talked about here, and then fix everything else you can think of, too) and then repost.



          After you fix everything, but before you repost, read your code from top to bottom one more time! Find two more things that need fixing, and fix them. Then repost.






          share|improve this answer









          $endgroup$





















            2












            $begingroup$

            I'll build off of Quuxplusone's answer.





            You have issues with tab.



            int tab[N];
            tab[0] = N-1;
            tab[N+1] = 0; // out of bounds


            Only the elements 0 through N-1 are available, so this assignment is wrong. Your initialization loop also attempts to initialize tab[N]. And in flip, since a and b are randomly generated between 0 and N-1, getting tab[a+2] can be N+1 at maximum, also out of bounds.



            While many compilers will accept it, you're not allowed to create an array with a non-const int. However, all these issues can be fixed pretty easily; just create it with a bit of extra room.



            const int N = 20; // set to const
            int tab[N+2];


            As a side note, I would personally not use tab at all! I would simply do the wraparound logic within flip():



            int val1 = lattice[a > 0 ? a-1 : N-1][b];
            int val2 = lattice[a < N-1 ? a+1 : 0][b];
            int val3 = lattice[a][b > 0 ? b-1 : N-1];
            int val4 = lattice[a][b < N-1 ? b+1 : 0];


            That way, you don't have to worry about creating it or passing it as a parameter at all.





            Your flip function needlessly copies lattice each call. You should pass it as a reference.



            void flip (int N, std::vector<std::vector<int>>& lattice, float beta, int tab, float H)
            // ^
            {
            ...
            return;
            }



            I've also made the function return void since you don't need to reassign lattice since its passed by reference and just call it like so:



            flip(N, lattice, beta, tab, H);




            Make a separate function like print_lattice. It makes it clear whats happening without even looking at it and you can use it in multiple places (like the one you've commented out).





            Don't declare your loop variables outside the loop (unless you need to). And get rid of any unused variables.



            inta,b,N=20,i,j,k,r,t,sweep=1500;





            Use better variable names. Consider using x and y instead of a and b since they'd be more immediately understandable. And give a more descriptive name to H and M, I'm not familiar with the algorithm and these names don't help much.





            for(int u=0;u<N;u++)
            {
            if(i>=500)
            {M_sweep=M_sweep+lattice[t][u];}
            }


            This loop checks for i but never modifies it, this check can be moved outside the loop like so:



            if (i >= 500)
            {
            for(int u=0;u<N;u++)
            {
            M_sweep = M_sweep + lattice[t][u];
            }
            }


            On second look, it can moved even higher to outside the t loop.





            You use lots of constants:



            H=H+0.015;
            ...
            T=2.2;


            Consider giving these constants names.



            const float H_STEP = 0.015f;





            share|improve this answer









            $endgroup$














              Your Answer





              StackExchange.ifUsing("editor", function () {
              return StackExchange.using("mathjaxEditing", function () {
              StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
              StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
              });
              });
              }, "mathjax-editing");

              StackExchange.ifUsing("editor", function () {
              StackExchange.using("externalEditor", function () {
              StackExchange.using("snippets", function () {
              StackExchange.snippets.init();
              });
              });
              }, "code-snippets");

              StackExchange.ready(function() {
              var channelOptions = {
              tags: "".split(" "),
              id: "196"
              };
              initTagRenderer("".split(" "), "".split(" "), channelOptions);

              StackExchange.using("externalEditor", function() {
              // Have to fire editor after snippets, if snippets enabled
              if (StackExchange.settings.snippets.snippetsEnabled) {
              StackExchange.using("snippets", function() {
              createEditor();
              });
              }
              else {
              createEditor();
              }
              });

              function createEditor() {
              StackExchange.prepareEditor({
              heartbeatType: 'answer',
              autoActivateHeartbeat: false,
              convertImagesToLinks: false,
              noModals: true,
              showLowRepImageUploadWarning: true,
              reputationToPostImages: null,
              bindNavPrevention: true,
              postfix: "",
              imageUploader: {
              brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
              contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
              allowUrls: true
              },
              onDemand: true,
              discardSelector: ".discard-answer"
              ,immediatelyShowMarkdownHelp:true
              });


              }
              });






              aargiee is a new contributor. Be nice, and check out our Code of Conduct.










              draft saved

              draft discarded


















              StackExchange.ready(
              function () {
              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216607%2fising-model-simulation%23new-answer', 'question_page');
              }
              );

              Post as a guest















              Required, but never shown

























              2 Answers
              2






              active

              oldest

              votes








              2 Answers
              2






              active

              oldest

              votes









              active

              oldest

              votes






              active

              oldest

              votes









              3












              $begingroup$

              There are many, many things wrong with your code; I'll list some, and then I advise you to fix as many of them as you can (and more) and then repost a new question with the revised code.



              First of all, you should compile your code with -W -Wall -Wextra (or -W4 if you use MSVC). Read the first warning diagnostic. Fix it. Recompile. Read the first diagnostic. Fix it. ...until all the warnings are completely gone.



              Procedural nit: When you post code to CodeReview, please post it in one big cut-and-pasteable chunk, or at least just a couple of chunks. Your current post mixes code and commentary in a way that makes it hard for the reviewer to cut-and-paste the whole piece of code into a file for testing.





              Speaking of commentary:



                  float T, beta;
              //beta=1.0/T; // boltzman constant is assumed to be 1.
              //creating a 2d lattice and populating it
              std::vector< std::vector < int > >lattice;


              Is beta supposed to be 1.0 / T, or not? By commenting out that line of code, you make it invisible to the compiler. Better make it invisible to the reader, too, and just delete it! (If you're commenting things out to preserve the "history" of the code, read up on version control systems like git, and then use one. It's easy!)



              Furthermore, since you don't initialize beta, you can probably just get rid of the variable entirely.



              Finally, the idiomatic way to place the whitespace when you're defining a variable of type std::vector<std::vector<int>> is simply thus:



              std::vector<std::vector<int>> lattice;


              Notice the space between the variable's type and its name; and the lack of space anywhere else.





              Populating that lattice can be done quickly and easily using vector's "filling" constructor:



              std::vector<std::vector<int>> lattice(N, std::vector<int>(N, -1));


              Now you don't need those nested for loops anymore!





              Going back up to the top of your code:



              int spin(int r) 
              {
              int s;
              if(r>5)
              {
              s=+1;
              }
              else
              {
              s=-1;
              }

              return s;
              }


              Replace this 14-line function with a 4-line function that does the same thing more clearly and simply:



              int spin(int r)
              {
              return (r > 5) ? 1 : -1;
              }


              No local variables, no mutation, no startling use of the =+ "operator"; and perhaps most importantly for the reader, there's now 10 more lines available on my screen so I can look at other code at the same time! Vertical real estate can be important for reading comprehension.





              float prandom(int i,int N)
              {
              std::random_device rd; //Will be used to obtain a seed for the random number engine
              std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
              std::uniform_real_distribution<> dis(i,N);
              // Use dis to transform the random unsigned int generated by gen into a
              // double in [1, 2). Each call to dis(gen) generates a new random double
              int t = dis(gen);
              return t;
              }


              This is wrong in at least two ways. First, most importantly: you're constructing a std::random_device on each call to this function. That is extremely expensive. Think of std::random_device as an open file handle to /dev/urandom, because that's what it is, under the hood. That means every time you call prandom, you're opening a file, reading some bytes, and closing it again!



              You should keep a global random number generator, initialized just once at the start of the program. One way to do this (not cheap, but not as expensive as opening a file on every call to prandom) would be



              float prandom(int i,int N)
              {
              static std::mt19937 gen = () {
              std::random_device rd;
              return std::mt19937(rd());
              }();
              std::uniform_real_distribution<float> dis(i, N);
              // ...


              Notice that uniform_real_distribution<> is secretly an alias for uniform_real_distribution<double>. Your code doesn't use doubles; it uses floats. So it's (always) better to be explicit and say what type you mean — you have less chance of getting it wrong by accident!



                  int t = dis(gen);
              return t;


              ...And then you go ahead and stuff the return value into an int anyway! So what was the point of using a uniform_real_distribution in the first place? And what's the point of returning a float from prandom? Did you mean to simply



              return dis(gen);


              instead?



              You're also seeding your PRNG wrong, but seeding it correctly is a huge headache in C++17, so never mind that.





                      if(temp>15)
              {
              H=H-0.015;
              }


              Please indent your code correctly. You can use the clang-format command-line tool to automatically indent everything, or if you use a graphical IDE it almost certainly has an "Indent" option somewhere in the menus.





              That's enough for one day. As I said above, I advise you to fix as much as possible (that is, fix everything I talked about here, and then fix everything else you can think of, too) and then repost.



              After you fix everything, but before you repost, read your code from top to bottom one more time! Find two more things that need fixing, and fix them. Then repost.






              share|improve this answer









              $endgroup$


















                3












                $begingroup$

                There are many, many things wrong with your code; I'll list some, and then I advise you to fix as many of them as you can (and more) and then repost a new question with the revised code.



                First of all, you should compile your code with -W -Wall -Wextra (or -W4 if you use MSVC). Read the first warning diagnostic. Fix it. Recompile. Read the first diagnostic. Fix it. ...until all the warnings are completely gone.



                Procedural nit: When you post code to CodeReview, please post it in one big cut-and-pasteable chunk, or at least just a couple of chunks. Your current post mixes code and commentary in a way that makes it hard for the reviewer to cut-and-paste the whole piece of code into a file for testing.





                Speaking of commentary:



                    float T, beta;
                //beta=1.0/T; // boltzman constant is assumed to be 1.
                //creating a 2d lattice and populating it
                std::vector< std::vector < int > >lattice;


                Is beta supposed to be 1.0 / T, or not? By commenting out that line of code, you make it invisible to the compiler. Better make it invisible to the reader, too, and just delete it! (If you're commenting things out to preserve the "history" of the code, read up on version control systems like git, and then use one. It's easy!)



                Furthermore, since you don't initialize beta, you can probably just get rid of the variable entirely.



                Finally, the idiomatic way to place the whitespace when you're defining a variable of type std::vector<std::vector<int>> is simply thus:



                std::vector<std::vector<int>> lattice;


                Notice the space between the variable's type and its name; and the lack of space anywhere else.





                Populating that lattice can be done quickly and easily using vector's "filling" constructor:



                std::vector<std::vector<int>> lattice(N, std::vector<int>(N, -1));


                Now you don't need those nested for loops anymore!





                Going back up to the top of your code:



                int spin(int r) 
                {
                int s;
                if(r>5)
                {
                s=+1;
                }
                else
                {
                s=-1;
                }

                return s;
                }


                Replace this 14-line function with a 4-line function that does the same thing more clearly and simply:



                int spin(int r)
                {
                return (r > 5) ? 1 : -1;
                }


                No local variables, no mutation, no startling use of the =+ "operator"; and perhaps most importantly for the reader, there's now 10 more lines available on my screen so I can look at other code at the same time! Vertical real estate can be important for reading comprehension.





                float prandom(int i,int N)
                {
                std::random_device rd; //Will be used to obtain a seed for the random number engine
                std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
                std::uniform_real_distribution<> dis(i,N);
                // Use dis to transform the random unsigned int generated by gen into a
                // double in [1, 2). Each call to dis(gen) generates a new random double
                int t = dis(gen);
                return t;
                }


                This is wrong in at least two ways. First, most importantly: you're constructing a std::random_device on each call to this function. That is extremely expensive. Think of std::random_device as an open file handle to /dev/urandom, because that's what it is, under the hood. That means every time you call prandom, you're opening a file, reading some bytes, and closing it again!



                You should keep a global random number generator, initialized just once at the start of the program. One way to do this (not cheap, but not as expensive as opening a file on every call to prandom) would be



                float prandom(int i,int N)
                {
                static std::mt19937 gen = () {
                std::random_device rd;
                return std::mt19937(rd());
                }();
                std::uniform_real_distribution<float> dis(i, N);
                // ...


                Notice that uniform_real_distribution<> is secretly an alias for uniform_real_distribution<double>. Your code doesn't use doubles; it uses floats. So it's (always) better to be explicit and say what type you mean — you have less chance of getting it wrong by accident!



                    int t = dis(gen);
                return t;


                ...And then you go ahead and stuff the return value into an int anyway! So what was the point of using a uniform_real_distribution in the first place? And what's the point of returning a float from prandom? Did you mean to simply



                return dis(gen);


                instead?



                You're also seeding your PRNG wrong, but seeding it correctly is a huge headache in C++17, so never mind that.





                        if(temp>15)
                {
                H=H-0.015;
                }


                Please indent your code correctly. You can use the clang-format command-line tool to automatically indent everything, or if you use a graphical IDE it almost certainly has an "Indent" option somewhere in the menus.





                That's enough for one day. As I said above, I advise you to fix as much as possible (that is, fix everything I talked about here, and then fix everything else you can think of, too) and then repost.



                After you fix everything, but before you repost, read your code from top to bottom one more time! Find two more things that need fixing, and fix them. Then repost.






                share|improve this answer









                $endgroup$
















                  3












                  3








                  3





                  $begingroup$

                  There are many, many things wrong with your code; I'll list some, and then I advise you to fix as many of them as you can (and more) and then repost a new question with the revised code.



                  First of all, you should compile your code with -W -Wall -Wextra (or -W4 if you use MSVC). Read the first warning diagnostic. Fix it. Recompile. Read the first diagnostic. Fix it. ...until all the warnings are completely gone.



                  Procedural nit: When you post code to CodeReview, please post it in one big cut-and-pasteable chunk, or at least just a couple of chunks. Your current post mixes code and commentary in a way that makes it hard for the reviewer to cut-and-paste the whole piece of code into a file for testing.





                  Speaking of commentary:



                      float T, beta;
                  //beta=1.0/T; // boltzman constant is assumed to be 1.
                  //creating a 2d lattice and populating it
                  std::vector< std::vector < int > >lattice;


                  Is beta supposed to be 1.0 / T, or not? By commenting out that line of code, you make it invisible to the compiler. Better make it invisible to the reader, too, and just delete it! (If you're commenting things out to preserve the "history" of the code, read up on version control systems like git, and then use one. It's easy!)



                  Furthermore, since you don't initialize beta, you can probably just get rid of the variable entirely.



                  Finally, the idiomatic way to place the whitespace when you're defining a variable of type std::vector<std::vector<int>> is simply thus:



                  std::vector<std::vector<int>> lattice;


                  Notice the space between the variable's type and its name; and the lack of space anywhere else.





                  Populating that lattice can be done quickly and easily using vector's "filling" constructor:



                  std::vector<std::vector<int>> lattice(N, std::vector<int>(N, -1));


                  Now you don't need those nested for loops anymore!





                  Going back up to the top of your code:



                  int spin(int r) 
                  {
                  int s;
                  if(r>5)
                  {
                  s=+1;
                  }
                  else
                  {
                  s=-1;
                  }

                  return s;
                  }


                  Replace this 14-line function with a 4-line function that does the same thing more clearly and simply:



                  int spin(int r)
                  {
                  return (r > 5) ? 1 : -1;
                  }


                  No local variables, no mutation, no startling use of the =+ "operator"; and perhaps most importantly for the reader, there's now 10 more lines available on my screen so I can look at other code at the same time! Vertical real estate can be important for reading comprehension.





                  float prandom(int i,int N)
                  {
                  std::random_device rd; //Will be used to obtain a seed for the random number engine
                  std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
                  std::uniform_real_distribution<> dis(i,N);
                  // Use dis to transform the random unsigned int generated by gen into a
                  // double in [1, 2). Each call to dis(gen) generates a new random double
                  int t = dis(gen);
                  return t;
                  }


                  This is wrong in at least two ways. First, most importantly: you're constructing a std::random_device on each call to this function. That is extremely expensive. Think of std::random_device as an open file handle to /dev/urandom, because that's what it is, under the hood. That means every time you call prandom, you're opening a file, reading some bytes, and closing it again!



                  You should keep a global random number generator, initialized just once at the start of the program. One way to do this (not cheap, but not as expensive as opening a file on every call to prandom) would be



                  float prandom(int i,int N)
                  {
                  static std::mt19937 gen = () {
                  std::random_device rd;
                  return std::mt19937(rd());
                  }();
                  std::uniform_real_distribution<float> dis(i, N);
                  // ...


                  Notice that uniform_real_distribution<> is secretly an alias for uniform_real_distribution<double>. Your code doesn't use doubles; it uses floats. So it's (always) better to be explicit and say what type you mean — you have less chance of getting it wrong by accident!



                      int t = dis(gen);
                  return t;


                  ...And then you go ahead and stuff the return value into an int anyway! So what was the point of using a uniform_real_distribution in the first place? And what's the point of returning a float from prandom? Did you mean to simply



                  return dis(gen);


                  instead?



                  You're also seeding your PRNG wrong, but seeding it correctly is a huge headache in C++17, so never mind that.





                          if(temp>15)
                  {
                  H=H-0.015;
                  }


                  Please indent your code correctly. You can use the clang-format command-line tool to automatically indent everything, or if you use a graphical IDE it almost certainly has an "Indent" option somewhere in the menus.





                  That's enough for one day. As I said above, I advise you to fix as much as possible (that is, fix everything I talked about here, and then fix everything else you can think of, too) and then repost.



                  After you fix everything, but before you repost, read your code from top to bottom one more time! Find two more things that need fixing, and fix them. Then repost.






                  share|improve this answer









                  $endgroup$



                  There are many, many things wrong with your code; I'll list some, and then I advise you to fix as many of them as you can (and more) and then repost a new question with the revised code.



                  First of all, you should compile your code with -W -Wall -Wextra (or -W4 if you use MSVC). Read the first warning diagnostic. Fix it. Recompile. Read the first diagnostic. Fix it. ...until all the warnings are completely gone.



                  Procedural nit: When you post code to CodeReview, please post it in one big cut-and-pasteable chunk, or at least just a couple of chunks. Your current post mixes code and commentary in a way that makes it hard for the reviewer to cut-and-paste the whole piece of code into a file for testing.





                  Speaking of commentary:



                      float T, beta;
                  //beta=1.0/T; // boltzman constant is assumed to be 1.
                  //creating a 2d lattice and populating it
                  std::vector< std::vector < int > >lattice;


                  Is beta supposed to be 1.0 / T, or not? By commenting out that line of code, you make it invisible to the compiler. Better make it invisible to the reader, too, and just delete it! (If you're commenting things out to preserve the "history" of the code, read up on version control systems like git, and then use one. It's easy!)



                  Furthermore, since you don't initialize beta, you can probably just get rid of the variable entirely.



                  Finally, the idiomatic way to place the whitespace when you're defining a variable of type std::vector<std::vector<int>> is simply thus:



                  std::vector<std::vector<int>> lattice;


                  Notice the space between the variable's type and its name; and the lack of space anywhere else.





                  Populating that lattice can be done quickly and easily using vector's "filling" constructor:



                  std::vector<std::vector<int>> lattice(N, std::vector<int>(N, -1));


                  Now you don't need those nested for loops anymore!





                  Going back up to the top of your code:



                  int spin(int r) 
                  {
                  int s;
                  if(r>5)
                  {
                  s=+1;
                  }
                  else
                  {
                  s=-1;
                  }

                  return s;
                  }


                  Replace this 14-line function with a 4-line function that does the same thing more clearly and simply:



                  int spin(int r)
                  {
                  return (r > 5) ? 1 : -1;
                  }


                  No local variables, no mutation, no startling use of the =+ "operator"; and perhaps most importantly for the reader, there's now 10 more lines available on my screen so I can look at other code at the same time! Vertical real estate can be important for reading comprehension.





                  float prandom(int i,int N)
                  {
                  std::random_device rd; //Will be used to obtain a seed for the random number engine
                  std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
                  std::uniform_real_distribution<> dis(i,N);
                  // Use dis to transform the random unsigned int generated by gen into a
                  // double in [1, 2). Each call to dis(gen) generates a new random double
                  int t = dis(gen);
                  return t;
                  }


                  This is wrong in at least two ways. First, most importantly: you're constructing a std::random_device on each call to this function. That is extremely expensive. Think of std::random_device as an open file handle to /dev/urandom, because that's what it is, under the hood. That means every time you call prandom, you're opening a file, reading some bytes, and closing it again!



                  You should keep a global random number generator, initialized just once at the start of the program. One way to do this (not cheap, but not as expensive as opening a file on every call to prandom) would be



                  float prandom(int i,int N)
                  {
                  static std::mt19937 gen = () {
                  std::random_device rd;
                  return std::mt19937(rd());
                  }();
                  std::uniform_real_distribution<float> dis(i, N);
                  // ...


                  Notice that uniform_real_distribution<> is secretly an alias for uniform_real_distribution<double>. Your code doesn't use doubles; it uses floats. So it's (always) better to be explicit and say what type you mean — you have less chance of getting it wrong by accident!



                      int t = dis(gen);
                  return t;


                  ...And then you go ahead and stuff the return value into an int anyway! So what was the point of using a uniform_real_distribution in the first place? And what's the point of returning a float from prandom? Did you mean to simply



                  return dis(gen);


                  instead?



                  You're also seeding your PRNG wrong, but seeding it correctly is a huge headache in C++17, so never mind that.





                          if(temp>15)
                  {
                  H=H-0.015;
                  }


                  Please indent your code correctly. You can use the clang-format command-line tool to automatically indent everything, or if you use a graphical IDE it almost certainly has an "Indent" option somewhere in the menus.





                  That's enough for one day. As I said above, I advise you to fix as much as possible (that is, fix everything I talked about here, and then fix everything else you can think of, too) and then repost.



                  After you fix everything, but before you repost, read your code from top to bottom one more time! Find two more things that need fixing, and fix them. Then repost.







                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered 2 hours ago









                  QuuxplusoneQuuxplusone

                  12.9k12062




                  12.9k12062

























                      2












                      $begingroup$

                      I'll build off of Quuxplusone's answer.





                      You have issues with tab.



                      int tab[N];
                      tab[0] = N-1;
                      tab[N+1] = 0; // out of bounds


                      Only the elements 0 through N-1 are available, so this assignment is wrong. Your initialization loop also attempts to initialize tab[N]. And in flip, since a and b are randomly generated between 0 and N-1, getting tab[a+2] can be N+1 at maximum, also out of bounds.



                      While many compilers will accept it, you're not allowed to create an array with a non-const int. However, all these issues can be fixed pretty easily; just create it with a bit of extra room.



                      const int N = 20; // set to const
                      int tab[N+2];


                      As a side note, I would personally not use tab at all! I would simply do the wraparound logic within flip():



                      int val1 = lattice[a > 0 ? a-1 : N-1][b];
                      int val2 = lattice[a < N-1 ? a+1 : 0][b];
                      int val3 = lattice[a][b > 0 ? b-1 : N-1];
                      int val4 = lattice[a][b < N-1 ? b+1 : 0];


                      That way, you don't have to worry about creating it or passing it as a parameter at all.





                      Your flip function needlessly copies lattice each call. You should pass it as a reference.



                      void flip (int N, std::vector<std::vector<int>>& lattice, float beta, int tab, float H)
                      // ^
                      {
                      ...
                      return;
                      }



                      I've also made the function return void since you don't need to reassign lattice since its passed by reference and just call it like so:



                      flip(N, lattice, beta, tab, H);




                      Make a separate function like print_lattice. It makes it clear whats happening without even looking at it and you can use it in multiple places (like the one you've commented out).





                      Don't declare your loop variables outside the loop (unless you need to). And get rid of any unused variables.



                      inta,b,N=20,i,j,k,r,t,sweep=1500;





                      Use better variable names. Consider using x and y instead of a and b since they'd be more immediately understandable. And give a more descriptive name to H and M, I'm not familiar with the algorithm and these names don't help much.





                      for(int u=0;u<N;u++)
                      {
                      if(i>=500)
                      {M_sweep=M_sweep+lattice[t][u];}
                      }


                      This loop checks for i but never modifies it, this check can be moved outside the loop like so:



                      if (i >= 500)
                      {
                      for(int u=0;u<N;u++)
                      {
                      M_sweep = M_sweep + lattice[t][u];
                      }
                      }


                      On second look, it can moved even higher to outside the t loop.





                      You use lots of constants:



                      H=H+0.015;
                      ...
                      T=2.2;


                      Consider giving these constants names.



                      const float H_STEP = 0.015f;





                      share|improve this answer









                      $endgroup$


















                        2












                        $begingroup$

                        I'll build off of Quuxplusone's answer.





                        You have issues with tab.



                        int tab[N];
                        tab[0] = N-1;
                        tab[N+1] = 0; // out of bounds


                        Only the elements 0 through N-1 are available, so this assignment is wrong. Your initialization loop also attempts to initialize tab[N]. And in flip, since a and b are randomly generated between 0 and N-1, getting tab[a+2] can be N+1 at maximum, also out of bounds.



                        While many compilers will accept it, you're not allowed to create an array with a non-const int. However, all these issues can be fixed pretty easily; just create it with a bit of extra room.



                        const int N = 20; // set to const
                        int tab[N+2];


                        As a side note, I would personally not use tab at all! I would simply do the wraparound logic within flip():



                        int val1 = lattice[a > 0 ? a-1 : N-1][b];
                        int val2 = lattice[a < N-1 ? a+1 : 0][b];
                        int val3 = lattice[a][b > 0 ? b-1 : N-1];
                        int val4 = lattice[a][b < N-1 ? b+1 : 0];


                        That way, you don't have to worry about creating it or passing it as a parameter at all.





                        Your flip function needlessly copies lattice each call. You should pass it as a reference.



                        void flip (int N, std::vector<std::vector<int>>& lattice, float beta, int tab, float H)
                        // ^
                        {
                        ...
                        return;
                        }



                        I've also made the function return void since you don't need to reassign lattice since its passed by reference and just call it like so:



                        flip(N, lattice, beta, tab, H);




                        Make a separate function like print_lattice. It makes it clear whats happening without even looking at it and you can use it in multiple places (like the one you've commented out).





                        Don't declare your loop variables outside the loop (unless you need to). And get rid of any unused variables.



                        inta,b,N=20,i,j,k,r,t,sweep=1500;





                        Use better variable names. Consider using x and y instead of a and b since they'd be more immediately understandable. And give a more descriptive name to H and M, I'm not familiar with the algorithm and these names don't help much.





                        for(int u=0;u<N;u++)
                        {
                        if(i>=500)
                        {M_sweep=M_sweep+lattice[t][u];}
                        }


                        This loop checks for i but never modifies it, this check can be moved outside the loop like so:



                        if (i >= 500)
                        {
                        for(int u=0;u<N;u++)
                        {
                        M_sweep = M_sweep + lattice[t][u];
                        }
                        }


                        On second look, it can moved even higher to outside the t loop.





                        You use lots of constants:



                        H=H+0.015;
                        ...
                        T=2.2;


                        Consider giving these constants names.



                        const float H_STEP = 0.015f;





                        share|improve this answer









                        $endgroup$
















                          2












                          2








                          2





                          $begingroup$

                          I'll build off of Quuxplusone's answer.





                          You have issues with tab.



                          int tab[N];
                          tab[0] = N-1;
                          tab[N+1] = 0; // out of bounds


                          Only the elements 0 through N-1 are available, so this assignment is wrong. Your initialization loop also attempts to initialize tab[N]. And in flip, since a and b are randomly generated between 0 and N-1, getting tab[a+2] can be N+1 at maximum, also out of bounds.



                          While many compilers will accept it, you're not allowed to create an array with a non-const int. However, all these issues can be fixed pretty easily; just create it with a bit of extra room.



                          const int N = 20; // set to const
                          int tab[N+2];


                          As a side note, I would personally not use tab at all! I would simply do the wraparound logic within flip():



                          int val1 = lattice[a > 0 ? a-1 : N-1][b];
                          int val2 = lattice[a < N-1 ? a+1 : 0][b];
                          int val3 = lattice[a][b > 0 ? b-1 : N-1];
                          int val4 = lattice[a][b < N-1 ? b+1 : 0];


                          That way, you don't have to worry about creating it or passing it as a parameter at all.





                          Your flip function needlessly copies lattice each call. You should pass it as a reference.



                          void flip (int N, std::vector<std::vector<int>>& lattice, float beta, int tab, float H)
                          // ^
                          {
                          ...
                          return;
                          }



                          I've also made the function return void since you don't need to reassign lattice since its passed by reference and just call it like so:



                          flip(N, lattice, beta, tab, H);




                          Make a separate function like print_lattice. It makes it clear whats happening without even looking at it and you can use it in multiple places (like the one you've commented out).





                          Don't declare your loop variables outside the loop (unless you need to). And get rid of any unused variables.



                          inta,b,N=20,i,j,k,r,t,sweep=1500;





                          Use better variable names. Consider using x and y instead of a and b since they'd be more immediately understandable. And give a more descriptive name to H and M, I'm not familiar with the algorithm and these names don't help much.





                          for(int u=0;u<N;u++)
                          {
                          if(i>=500)
                          {M_sweep=M_sweep+lattice[t][u];}
                          }


                          This loop checks for i but never modifies it, this check can be moved outside the loop like so:



                          if (i >= 500)
                          {
                          for(int u=0;u<N;u++)
                          {
                          M_sweep = M_sweep + lattice[t][u];
                          }
                          }


                          On second look, it can moved even higher to outside the t loop.





                          You use lots of constants:



                          H=H+0.015;
                          ...
                          T=2.2;


                          Consider giving these constants names.



                          const float H_STEP = 0.015f;





                          share|improve this answer









                          $endgroup$



                          I'll build off of Quuxplusone's answer.





                          You have issues with tab.



                          int tab[N];
                          tab[0] = N-1;
                          tab[N+1] = 0; // out of bounds


                          Only the elements 0 through N-1 are available, so this assignment is wrong. Your initialization loop also attempts to initialize tab[N]. And in flip, since a and b are randomly generated between 0 and N-1, getting tab[a+2] can be N+1 at maximum, also out of bounds.



                          While many compilers will accept it, you're not allowed to create an array with a non-const int. However, all these issues can be fixed pretty easily; just create it with a bit of extra room.



                          const int N = 20; // set to const
                          int tab[N+2];


                          As a side note, I would personally not use tab at all! I would simply do the wraparound logic within flip():



                          int val1 = lattice[a > 0 ? a-1 : N-1][b];
                          int val2 = lattice[a < N-1 ? a+1 : 0][b];
                          int val3 = lattice[a][b > 0 ? b-1 : N-1];
                          int val4 = lattice[a][b < N-1 ? b+1 : 0];


                          That way, you don't have to worry about creating it or passing it as a parameter at all.





                          Your flip function needlessly copies lattice each call. You should pass it as a reference.



                          void flip (int N, std::vector<std::vector<int>>& lattice, float beta, int tab, float H)
                          // ^
                          {
                          ...
                          return;
                          }



                          I've also made the function return void since you don't need to reassign lattice since its passed by reference and just call it like so:



                          flip(N, lattice, beta, tab, H);




                          Make a separate function like print_lattice. It makes it clear whats happening without even looking at it and you can use it in multiple places (like the one you've commented out).





                          Don't declare your loop variables outside the loop (unless you need to). And get rid of any unused variables.



                          inta,b,N=20,i,j,k,r,t,sweep=1500;





                          Use better variable names. Consider using x and y instead of a and b since they'd be more immediately understandable. And give a more descriptive name to H and M, I'm not familiar with the algorithm and these names don't help much.





                          for(int u=0;u<N;u++)
                          {
                          if(i>=500)
                          {M_sweep=M_sweep+lattice[t][u];}
                          }


                          This loop checks for i but never modifies it, this check can be moved outside the loop like so:



                          if (i >= 500)
                          {
                          for(int u=0;u<N;u++)
                          {
                          M_sweep = M_sweep + lattice[t][u];
                          }
                          }


                          On second look, it can moved even higher to outside the t loop.





                          You use lots of constants:



                          H=H+0.015;
                          ...
                          T=2.2;


                          Consider giving these constants names.



                          const float H_STEP = 0.015f;






                          share|improve this answer












                          share|improve this answer



                          share|improve this answer










                          answered 1 hour ago









                          kmdrekokmdreko

                          1412




                          1412






















                              aargiee is a new contributor. Be nice, and check out our Code of Conduct.










                              draft saved

                              draft discarded


















                              aargiee is a new contributor. Be nice, and check out our Code of Conduct.













                              aargiee is a new contributor. Be nice, and check out our Code of Conduct.












                              aargiee is a new contributor. Be nice, and check out our Code of Conduct.
















                              Thanks for contributing an answer to Code Review Stack Exchange!


                              • Please be sure to answer the question. Provide details and share your research!

                              But avoid



                              • Asking for help, clarification, or responding to other answers.

                              • Making statements based on opinion; back them up with references or personal experience.


                              Use MathJax to format equations. MathJax reference.


                              To learn more, see our tips on writing great answers.




                              draft saved


                              draft discarded














                              StackExchange.ready(
                              function () {
                              StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216607%2fising-model-simulation%23new-answer', 'question_page');
                              }
                              );

                              Post as a guest















                              Required, but never shown





















































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown

































                              Required, but never shown














                              Required, but never shown












                              Required, but never shown







                              Required, but never shown







                              Popular posts from this blog

                              Knooppunt Holsloot

                              Altaar (religie)

                              Gregoriusmis