Not getting the correct vector

[kbklpl21]

I'm comparing values of a certain equation and I need to store the best value in a certain variable, in my case current_best_eval , and the vector that leads me to the value, ILS_best_p , however in my if statements, there seem to be a problem fetching the correct vector, the vector keeps changing until the vector of the final iteration instead of getting the best vector although I manage to get the best value from the same if statement, so I was hoping if someone can point out where the if statement needs to be corrected. This is the main function of my programming, if necessary I'll post the whole coding. Thanks in advance!

int main()
{
	string name;
	long int optimum;
	ifstream infile;
	long int initial_eval;
	long int *ILS_best_p;
	long int new_vs_current;
	new_vs_current = INT_MAX;
	long int current_best_eval;
	current_best_eval = INT_MAX;
	long int new_current_eval;
	long int best_eval;
	long int *best_p;
	long int improvement;
	long int i;
	long int j;

	infile.open("nug12.dat");
	cout << "Name: ";
	infile >> name;
	cout << name << "\n";

	cout << "Rows and Columns: ";
	infile >> n;
	cout << n << "\n";

	cout << "Optimum solution: ";
	infile >> optimum;
	cout << optimum<<"\n";

	d=read_matrix(infile, n);
	f=read_matrix(infile, n);
	cout << endl;

	p=generate_random_vector(n); 
	compute_evaluation_function(n);
	cout << endl;
	first_2_opt_symmetric (n);
	cout << endl;	
	initial_eval = compute_evaluation_function(n);
	cout << endl << endl;
	
	for (i = 0; i < 500; i++) //ILS loop
	{
		if (new_eval < current_eval)
		{
			new_vs_current = new_eval;
			cout << "iteration 1st = " << i+1 << " " << endl;	
		}
		else if (current_eval < new_eval)
		{
			new_vs_current = current_eval;
			cout << "iteration 2nd = " << i+1 << " " << endl;			
		}
		cout << "iteration = " << i+1 << " " << endl;
		cout << " Vector before perturbation = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		perturbation(3); 
		cout << endl << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		cout << " Vector after perturbation = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl << endl;    
		 
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		first_2_opt_symmetric (n);
		
		new_current_eval = compute_evaluation_function(n);
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		cout << endl;
				
		cout << " Vector after local search = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		if (new_current_eval < current_best_eval)
		{
			new_eval = new_current_eval;
			//ILS_best_p = p;
			cout << "iteration 3rd = " << i+1 << " " << endl;
		}
		else if (current_best_eval < new_current_eval)
		{
			new_eval = current_best_eval;
			cout << "iteration 4th = " << i+1 << " " << endl;			
		}
		
		if (new_eval < new_vs_current)
		{
			current_best_eval = new_eval;
			cout << "iteration 5th = " << i+1 << " " << endl;
			ILS_best_p = p;			
		}
		else if (new_vs_current < new_eval)
		{
			current_best_eval = new_vs_current;
			cout << "iteration 6th = " << i+1 << " " << endl;	
			ILS_best_p = p;		
		}

		cout << endl << "current best eval = " << current_best_eval << endl; 
	}
	cout << endl << "current best eval = " << current_best_eval << endl;

	cout << "ILS best vector = ";
	for (i = 0; i < n; i++)
	{
		cout << ILS_best_p[i] << " ";
	}

	return 0;
}

[Handy Andy]

Hello kbklpl21,

Post the missing parts so the code can be compiled and tested. Also post the input file, or a good sample if large, so everyone can use the same information and see how the file is laid out without having to guess at what you are using.

Several places you have "cout" statements making a reference to a vector, but I do not see where you defined any vector, Only pointers that look like they may be used with a C style array, but even that is not created. Unless I have missed something. Your code is a little hard to read.

On line 19 you open a file stream, but how do you know it opened and is usable?

The code that follows can be shortened to:

infile >> name;
cout << "Name: " << name << "\n";

And the same for the others.

You can start with that while I take a deeper look at your program.

Andy

[kbklpl21]

It seems that the code is too long so I'll have to post it in the replies

[kbklpl21]

#include <iostream>
#include <string>
#include <fstream>
#include <cstdlib>
#include <algorithm>
#include <vector>
#include <cassert>
#include <ctime>
#include <cmath>

using namespace std;

long int **read_matrix(ifstream &input, long int n);
double ran01(long int *idum);
long int *generate_random_vector(long int n);
unsigned long int compute_evaluation_function(long int n);
void first_2_opt_symmetric (long int n);
void swap(long int pos_1, long int pos_2, long int *);
void perturbation(long int pert_size);
long int *p;
long int **d;
long int **f;
long int n;
long int actual_solution_value;
long int new_eval;
long int current_eval;

#define IA 16807
#define IM 2147483647
#define AM (1.0/IM)
#define IQ 127773
#define IR 2836

int main()
{
	string name;
	long int optimum;
	ifstream infile;
	long int initial_eval;
	long int *ILS_best_p;
	long int new_vs_current;
	new_vs_current = INT_MAX;
	long int current_best_eval;
	current_best_eval = INT_MAX;
	long int new_current_eval;
	long int best_eval;
	long int *best_p;
	long int improvement;
	long int i;
	long int j;

	infile.open("nug12.dat");
	infile >> name;
	cout << "Name: " << name << "\n";

	infile >> n;
	cout << "Rows and Columns: " << n << "\n";

	infile >> optimum;
	cout << "Optimum solution: " << optimum <<"\n";

	d=read_matrix(infile, n);
	f=read_matrix(infile, n);
	cout << endl;

	p=generate_random_vector(n); 
	compute_evaluation_function(n);
	cout << endl;
	first_2_opt_symmetric (n);
	cout << endl;	
	initial_eval = compute_evaluation_function(n);
	cout << endl << endl;
	
	for (i = 0; i < 500; i++) //ILS loop
	{
		if (new_eval < current_eval)
		{
			new_vs_current = new_eval;
			cout << "iteration 1st = " << i+1 << " " << endl;	
		}
		else if (current_eval < new_eval)
		{
			new_vs_current = current_eval;
			cout << "iteration 2nd = " << i+1 << " " << endl;			
		}
		cout << "iteration = " << i+1 << " " << endl;
		cout << " Vector before perturbation = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		perturbation(3); 
		cout << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		cout << " Vector after perturbation = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl;   
		 
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		first_2_opt_symmetric (n);
		
		new_current_eval = compute_evaluation_function(n);
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		cout << endl;
				
		cout << " Vector after local search = ";
		for (j = 0; j < n; j++)
		{
			cout << p[j] << " ";
		}
		cout << endl;
		cout << endl << "current best eval = " << current_best_eval << endl; 
		
		if (new_current_eval < current_best_eval)
		{
			new_eval = new_current_eval;
			cout << "iteration 3rd = " << i+1 << " " << endl;
		}
		else if (current_best_eval < new_current_eval)
		{
			new_eval = current_best_eval;
			cout << "iteration 4th = " << i+1 << " " << endl;			
		}
		
		if (new_eval < new_vs_current)
		{
			current_best_eval = new_eval;
			cout << "iteration 5th = " << i+1 << " " << endl;
			ILS_best_p = p;			
		}
		else if (new_vs_current < new_eval)
		{
			current_best_eval = new_vs_current;
			cout << "iteration 6th = " << i+1 << " " << endl;	
			ILS_best_p = p;		
		}

		cout << endl << "current best eval = " << current_best_eval << endl; 
	}
	cout << endl << "current best eval = " << current_best_eval << endl;

	cout << "ILS best vector = ";
	for (i = 0; i < n; i++)
	{
		cout << ILS_best_p[i] << " ";
	}

	return 0;
}

long int **read_matrix(ifstream &input, long int n)
{

    /*    
    FUNCTION:       read instance matrices
    INPUT:          instance file name, instance size
    OUTPUT:         d-matrix and f-matrix
    (SIDE)EFFECTS:  none
    COMMENTS:       read the distance matrix and flow matrix
    */

 
	long int i, j;
	long int **matrix = new long int *[n];
	for (i = 0; i < n; i++)
	{
		matrix[i] = new long int[n];
    	for (j = 0; j < n; j++)
		{
			if( !(input >> matrix[i][j]) )
			{
				cerr << "Error reading at " << i << j << endl;
			exit(1);
			}
    	}
	}
    return matrix;
}

double ran01(long int *idum)
{

    /*
    FUNCTION:      returns a pseudo-random number
    INPUT:         a pointer to the seed variable
    OUTPUT:        a pseudo-random number uniformly distributed in [0,1]
    (SIDE)EFFECTS: changes the value of seed
    */

 
	long k;
	double ans;

	k =(*idum)/IQ;
	*idum = IA * (*idum - k * IQ) - IR * k;
	if (*idum < 0 )
	{
		*idum += IM;
	}
    ans = AM * (*idum);
	return ans;
}

long int *generate_random_vector(long int n)
{
   
	/*    
    FUNCTION:      generates a random vector
    INPUT:         vector dimension
    OUTPUT:        returns pointer to vector, free memory when vector is not needed anymore
    (SIDE)EFFECTS: none
    */

 
	long int  i, j, help;
	long int  *v;
	srand(time(0));
	long int seed=rand();

	v = new long int[ n ];

	for ( i = 0 ; i < n; i++ )
	{
		v[i] = i;
	}

	for ( i = 0 ; i < n-1 ; i++)
	{
    	j = (long int) ( ran01( &seed ) * (n - i));
    	assert( i + j < n );
    	help = v[i];
    	v[i] = v[i+j];
    	v[i+j] = help;
  	}
  	return v;
}

unsigned long int compute_evaluation_function(long int n)
{

	/*
	FUNCTION:      compute evaluation function
	INPUT:         pointer to solution
	OUTPUT:        evaluation function
	(SIDE)EFFECTS: none
	COMMENTS:      none
    */

	long int i, j;
	unsigned long obj_f_value;

	obj_f_value = 0;
	for(i = 0; i < n; i++)
	{
		for(j = 0; j < n; j++)
		{
			obj_f_value += d[i][j] * f[p[i]][p[j]];
    	}
  	}
  	cout << obj_f_value;
	return obj_f_value;
}

void first_2_opt_symmetric (long int n)
{
	/*    
  	FUNCTION:      first improvement 2-opt local search for symmetric instances
  	INPUT:         pointer to some initial assignment
  	OUTPUT:        none
  	(SIDE)EFFECTS: initial assignment is locally optimized, dlb is used to increase the search speed, the dlb value is changed to false if item change it's location during perturbation
  	COMMENT:       neighborhood is scanned in random order, use of register for faster computation
	*/

	long int improvement = 1;
	long int improve_item = 0;
	long int u, v, i, j, k;
	long int tmp;
	long int *x;  
 
	improvement = 1;
	x = generate_random_vector(n);

	while (improvement)
	{
		improvement = 0;
    	for ( i = 0 ; i < n ; i++ )
		{
      		u = x[i];
      		improve_item = 0;
      		for ( j = 0 ; j < n ; j++ )
			{
        		v = x[j];
        		if (u == v)
          			continue;
        		tmp = 0;
        		for ( k = 0 ; k < n ; k++ )
				{
					if ( (k != u) && (k != v) )
					{
            			tmp += (d[u][k] - d[v][k]) * (f[p[v]][p[k]] - f[p[u]][p[k]]);
          			}    
        		}
        		if (tmp < 0)
				{
					improvement = 1;
          			improve_item = 1;    
          			swap (u, v, p);
        		}
      		}
		}
	}
	delete []x;
}

void swap(long int pos_1, long int pos_2, long int *p)
{

    /*
    FUNCTION:      swap position of two items in a vector
    INPUT:         position 1, position 2, pointer to vector
    OUTPUT:        none
    (SIDE)EFFECTS: none
    COMMENTS:      none
    */

	long int tmp;

	tmp = p[pos_1];
	p[pos_1] = p[pos_2];
  	p[pos_2] = tmp;
}

[kbklpl21]

void perturbation(long int pert_size)
{
	/*    
    FUNCTION:         apply random perturbation
    INPUT:            pointer to solution, perturbation scheme, perturbation strength, change in perturbation, end perturbation size
    OUTPUT:           none
    (SIDE)EFFECTS:    new solution formed from old solution
    COMMENTS:         none
    */

	long int hold_value[n];			//allocate memory for items to be chosen in move/*
	int chosen[pert_size];			//*allocate temporary memory to determine items to be moved */*
	long int i;
   
	for(i = 0; i < n; i++)
	{
    	hold_value[i] = p[i];				//initialize hold_value with the same value from local search/*
	}
  	int j = n - 1;
	int rand_number;
  	int rand_no;
   
  	for(i = 1; i <= pert_size; i++)
	{
		rand_number = rand();
    	rand_no = rand_number % j;				//choose random number from 1 to size - 1/
    	chosen[i - 1] = rand_no + i;			//copy the value to chosen[i]
    	j--;
  	}	
  	long int temp;
  	
  	for(i = 0; i < pert_size; i++)
	{
    	temp = chosen[i];
		swap(i, temp, hold_value);				//swap chosen[i] and hold_value[i]; n first item in hold_value[i] is the index array that will be included in perturbation/
  	}

	long int temp1;
	long int temp2;
	temp1 = p[hold_value[1]];//
	temp2 = p[hold_value[0]];

	for(i = 0; i < pert_size - 1; i++)
	{
    	p[hold_value[i + 1]] = temp2;
    	temp2 = temp1;
    	temp1 = p[hold_value[i + 2]];
  	}

  	p[hold_value[0]] = temp2;

	actual_solution_value = compute_evaluation_function(n);
  	new_eval = actual_solution_value;
  	current_eval = new_eval;
}

And this is the file I read from

Nug12
12
578
0 1 2 3 1 2 3 4 2 3 4 5
1 0 1 2 2 1 2 3 3 2 3 4
2 1 0 1 3 2 1 2 4 3 2 3
3 2 1 0 4 3 2 1 5 4 3 2
1 2 3 4 0 1 2 3 1 2 3 4
2 1 2 3 1 0 1 2 2 1 2 3
3 2 1 2 2 1 0 1 3 2 1 2
4 3 2 1 3 2 1 0 4 3 2 1
2 3 4 5 1 2 3 4 0 1 2 3
3 2 3 4 2 1 2 3 1 0 1 2
4 3 2 3 3 2 1 2 2 1 0 1
5 4 3 2 4 3 2 1 3 2 1 0

0 5 2 4 1 0 0 6 2 1 1 1
5 0 3 0 2 2 2 0 4 5 0 0
2 3 0 0 0 0 0 5 5 2 2 2
4 0 0 0 5 2 2 10 0 0 5 5
1 2 0 5 0 10 0 0 0 5 1 1
0 2 0 2 10 0 5 1 1 5 4 0
0 2 0 2 0 5 0 10 5 2 3 3
6 0 5 10 0 1 10 0 0 0 5 0
2 4 5 0 0 1 5 0 0 0 10 10
1 5 2 0 5 5 2 0 0 0 5 0
1 0 2 5 1 4 3 5 10 5 0 2
1 0 2 5 1 0 3 0 10 0 2 0

[kbklpl21]

Hi Andy, this code is the first time I'm dealing with reading from file so as you can see the code that's supposed to take 2 lines take 3 lines for me instead, I've changed the mentioned part.

Most parts of the code is from C from my lecturer, and I tried my best to change them to C++, of course with some help from others that responded to my question before, but I wouldn't doubt if there are still some parts that seems similar to C.

Regarding your question about whether I successfully opened the file, I didn't know as I don't do the check error thing, but since the code is running and the matrices and other data from the file are successfully read and my lecturer doesn't point that out, I just ignored it.

[Handy Andy]

Hello kbklpl21,

Thank you for the input file. That helps greatly.

When people have code that is to large for one reply they either do what you have done of use Pastebin https://pastebin.com/

A quick look at the program I noticed the global variables. Try to avoid global variables as any function in the file can change them and then it is hard to track down where it went wrong. Better to define these variables in "main" or in the functions where they are needed. This gives yo better control of your variables.

The "#define"s work, but from C++11 on, as an example constexpr variableType variableName{value};. Before C++11 just use "const".

The rest I will have to work through the program.

Andy

[kbklpl21]

Alright, thanks for the suggestion, I'll try looking in it next time!
Yes I know we should try avoiding global variables, but since those variables are used in several functions, I have them as global variables.

[Handy Andy]

Hello kbklpl21,

Yes I know we should try avoiding global variables, but since those variables are used in several functions, I have them as global variables.

You can define these global variables in "main" and pass them to the functions that need them. Even a variable defined in "main" and passed to a function can be passed to another function.

The more I look at it I seem to recall saying something about not using the "#define"s before. I could be think of something else as I have not looked back at what I have done.

The function "generate_random_vector" a very misleading name. Nowhere in the function do you generate a vector, but you do create a dynamic array. Even when I put a comment on "#include <vector>" it did not generate any errors which tells me that you are not using any vectors in the program.

In the "perturbation" function you have 2 lines of code:

long int hold_value[n];			//allocate memory for items to be chosen in move/*
int chosen[pert_size];			//*allocate temporary memory to determine items to be moved */* 

A VLA, variable length array, is not allowed in C++. The size of the array must be a constant at compile time so the compiler can set aside the proper amount of memory.

Another thing I noticed is that you have created several variables as pointers, but none of them are initialized and I am not sure if some of them ever receive a value.

I did have to make these changes just to get the code to compile with out errors. In "main":

long int *ILS_best_p{ nullptr };

if (ILS_best_p != nullptr)	
{
	for (i = 0; i < n; i++)
	{
		cout << ILS_best_p[i] << " ";
	}
}

Line 1 should be done. Everything after that, the if statement, is a work around just to get the code to compile and not meant to be a proper solution.

Another thing I changed in "main" is:

//ifstream infile("nug12.dat");  // <---An alternative to the open statement.

infile.open("nug12.dat");  // <--- Not needed with the above line.

if (!infile)
{
	std::cout << "\n    File \"nug12.dat\" did not open!\n";
		
	return 1;  // <--- No reason to continue until the problem is fixed.
}

srand(static_cast<size_t>(time(nullptr)));  // <--- Only needs done once. 

Now that I have the code compiling I can see what it is doing.

I was wondering is the point of the program to use pointers and dynamic memory or vectors?

Andy

[kbklpl21]

Ahh, yes some of the includes are extras from when I was trying out things, like the #include <vector>, I was trying to use vectors instead of arrays and I forgot to remove it.

About the name of the function, since what I'm dealing with was optimisation and mathematics, I straightforwardly called it a vector instead of array.

size of the array must be a constant at compile time

So even though the program would find out that n=12 and pert_size=3, writing the code like that is incorrect? Because if I were to use the code to read from another file, for instance with 30*30 matrices, I wouldn't want to change the coding just for another question.

if some of them ever receive a value

Doesn't being able to print them mean they have received the value wanted?

I did have to make these changes just to get the code to compile with out errors

By error do you mean you failed to compile? Because I can compile the code without errors shown from the return value. Also, I'm interested in this line long int *ILS_best_p{ nullptr };, why is the function or meaning of {nullptr}?

srand(static_cast<size_t>(time(nullptr)));
Is this line meant for generating a random vector? If so, can you explain the line? Is the static_cast the name of the vector variable? What does time(nullptr) do?

I was wondering is the point of the program to use pointers and dynamic memory or vectors?

My lecturer wanted me to use pointers, and most of the coding is from him, but in C, so I don't know whether the pointers need initialisation or anything of the sort.

Sorry for asking so many questions, my syllabus didn't teach about vectors, pointers and reading from files, also I didn't study C.

[Handy Andy]

Hello kbklpl21,

I understand what you are saying about the header file "vector" that just happened to be the one that caught my attention. I notice that some of the others may not be needed, but I have not looked into those yet.

So even though the program would find out that n=12 and pert_size=3, writing the code like that is incorrect?

Yes.

"n" and "pert_size" are variables that receive a value after the program runs. Since the compiler will create the array on the stack it need to know how much room to set aside at compile time. This can not happen with a variable the can change.

The other option is to use these variables to create a dynamic array on the heap which is allowed.

To use "n" and "pert_size" they would have to be defined as constexpr int n{ 12 }, pert_size{ 3 };, but this means that they can not be changed.

if some of them ever receive a value

Doesn't being able to print them mean they have received the value wanted?

Yes. I think you or maybe the way I put was misunderstood. I have not followed every pointer through the program and was unsure if they all received a proper value. Sorry for any confusion.

By error do you mean you failed to compile? Because I can compile the code without errors shown from the return value. Also, I'm interested in this line long int *ILS_best_p{ nullptr };, why is the function or meaning of {nullptr}?

Yes it failed to compile in my VS 2017. Keep in mind that there are differences between the IDE and compilers. What worked for you did not work for me and vice versa. Two possibilities are that your header files and compiler will let you do things that mine will not or your compiler is old and allows something that a newer version may not.

Two things here first the type "long int" the "int" part is not needed. Unless you define a "long double" "long" applies to "int"s. There was a time when (0) zero or "NULL" would have been used, but since C++11 on "nullptr" is the better choice to set a pointer variable to something that would be considered empty. This is one that you should get use to seeing and using.

srand(static_cast<size_t>(time(nullptr)));
"srand" is a C function used to seed the random number generator, (RNG). If you look up the function you will see that "srand" needs an "unsigned int" as its parameter. "size_t" is an alias for "unsigned int". If you look up "time" you will find the the number returned is not an "unsigned int" hence the type casting "static_cast<size_t>" and this is what should be used from C++11 on. Although I believe that the older forms of type casting still work. Although when calling "time" (0) zero and "NULL" may still work, but "nullptr" is the best choice.

This video is worth watching. At the least it will give you a better idea of how "rand" works. https://channel9.msdn.com/Events/GoingNative/2013/rand-Considered-Harmful and for something to read https://web.archive.org/web/20180123103235/http://cpp.indi.frih.net/blog/2014/12/the-bell-has-tolled-for-rand/

For the most part it looks like you have changed the C code into C++ well.

Now that I know what needs to be done I will see what I can do.

Andy

[kbklpl21]

https://web.archive.org/web/20180123103235/http://cpp.indi.frih.net/blog/2014/12/the-bell-has-tolled-for-rand/
This link seems to tell that seeding using time is not good as well, and I can't understand the things mentioned in C++ random library, so what should be done to be "good enough"? Or is srand(static_cast<size_t>(time(nullptr))); already good enough? If so, where should I use it? In the function that requires seeding or just in the main function?

set a pointer variable to something that would be considered empty

Does this mean the pointer variable has the properties of a dynamic array or just that the pointer variable contains 0 for all elements?

So, for now the things I understand and need to change so far are the #define's' to const and long int's' to long, am I right? And also trying to change the global variables to local variables (which would prove to be troublesome haha).

[Handy Andy]

Hello kbklpl21,

For now using "srand" and "rand" is good enough and will work.

I put the srand(static_cast<size_t>(time(nullptr))); in "main" like this:

long int improvement;
long int i;
long int j;
ifstream infile;

infile.open("nug12.dat");

if (!infile)
{
	std::cout << "\n    File \"nug12.dat\" did not open!\n";
		
	return 1;
}

srand(static_cast<size_t>(time(nullptr)));  // <--- Onlu needs done once.

infile >> name;
cout << "Name: " << name << "\n";

This way if you pass the if statement you would continue with the program and "srand" is the first code to process.

A variable defined as a pointer, e.g., long int *best_p; only holds an address to something. Should that something be an array the address is to the beginning of the array. It does not define an array. a line like: v = new long int[n]; defines a dynamic array and stored the address to the first element, the beginning of the array, in the pointer variable "v". At some point when you are finished with "v" you need a line of code delete[] v; to free the memory created with "new". This you must do or you will have a memory leak.

The "#define"s need to be fixed. Changing the "long int"s to just "long"s is not necessary. It does not matter either way. I was just pointing that out for the future.

I can see where moving the global variables is a big job. For now maybe it would be better to leave them or work on them slowly over time. Just knowing for now that you should avoid them and why is good enough and in future programs it will be easier to either define them in "main" and pass them to the functions that need them or just define them in the function that needs them.

Changing the "#define"s to constant variables and leaving them at the global level is fin because they can not be changed.

Andy

[kbklpl21]

Hello Andy,

Alright I'll put in the srand, so that means the srand in other functions can be removed right?

Should that something be an array the address is to the beginning of the array.

Ohhh so that's how it work, been looking online but no one explains it like this.

Hope you can help me find where does the code go wrong, which supposedly from the if statements saving the best vector to ILS_best_p should not have been a problem.

[Handy Andy]

Hello kbklpl21,

First question. Yes.

Start here https://www.learncpp.com/ and scroll down to chapter (P) about arrays. The explanations and examples there are very good.

I think I have worked out most of the problems with the pointers, but I am having a hard time trying to keep track of the single letter variable names. I waste a lot of time trying to figure them out and forget what I started working on.

Andy

[kbklpl21]

Hi Andy,

Sorry about the single letter variables, my lecturer suggested them and they prove to be easier to manipulate such as passing them as arguments stuffs. I understand for someone that didn't follow this project from the beginning, they could be hard to be remembered, so I'll name them in this reply. Sorry for the trouble.

d and f are both matrices read from the file and p is the vector generated randomly. In the process of the code, p should be have the vector that gives the lowest evaluation value, and what I want is to store that vector and the corresponding evaluation value into ILS_best_p and current_best_eval respectively, however although I've succeeded with storing the lowest evaluation value in current_best_eval, I couldn't seem to get the best vector into ILS_best_p (keeps storing the last vector). There shouldn't have been a problem since those two were in the same 'if' statement, so I'm in need of help to get this to work.

[Handy Andy]

Hello kbklpl21,

Consider that you lecturer has dealt with this program for some number of years and many students and there for use to the single letters. The size of this program is a good example of why a good variable name, something that describes what it is or what it does, can make a big difference in writing and following the variables in the program.

I think you are referring to this:

if (new_eval < new_vs_current)
{
	current_best_eval = new_eval;
	cout << "iteration 5th = " << i + 1 << " " << endl;
	ILS_best_p = randomArray;
}
else if (new_vs_current < new_eval)
{
	current_best_eval = new_vs_current;
	cout << "iteration 6th = " << i + 1 << " " << endl;
	ILS_best_p = randomArray;
}

From what I can tell "randomArray' was set with the function call to "generate_random_vector" that was done before the for loop. Each time you iterate through the for loop the if or else if sets "ILS_best_p = randomArray;" to the same address. Is this what you wanted or should "randomArray" be different in each iteration?

Sorry I have not worked on that part yet.

When looking through the for loop I did notice:
new_current_eval = compute_evaluation_function(numRowsCols);. "new_current_eval" is defined as a "long", but the function returns an "unsigned long". This may never be a problem, but it could.

I also noticed that there are many places where you have defined variables as "long"s where an "int" would work. Then there is:

void first_2_opt_symmetric(long int numRowsCols)
{
	long int improvement = 1;
	long int improve_item = 0;

These variables appear to hold either a (0) zero or a (1) and would be better defined as a "bool" which is 1 byte instead of wasting 3 or more bytes for a long.

My next step is to finish up what I was doing and start on the for loop. But as you said I was not there at the start and do not understand all that is needed for this program.

Andy

[kbklpl21]

Hi Andy,

should "randomArray" be different in each iteration

The "randomArray" should change every iteration after the call of perturbation and first_2_opt, perturbation serves to change the initially found "best randomArray" so that we are able to find an even better vector for the problem, and first_2_opt serves for permutation of the vector into the "best" vector possible (several iterations might be needed for this to be achieved), so the "randomArray" should be different every iteration, shouldn't these changes be updated to the array stored by the variable? What I was trying to achieve from this line ILS_best_p = randomArray; is to store the new vector obtained from the first_2_opt function (if it improves the value obtained) into ILS_best_p.

"new_current_eval" is defined as a "long", but the function returns an "unsigned long"

Ahh, I didn't notice the different type of the variables, since my compiler didn't warn me, so should I change new_current_eval to 'unsigned long' too?

	
long int improvement = 1;
long int improve_item = 0;

This was from my lecturer, which is in C, the original one was

long int improvement = TRUE;
long int improve_item = FALSE;

So I didn't thought of changing it to bool, if I change it to bool, nothing else needs to be changed right?