Your code is very poorly formatted, making it very hard to read. There are a lot of mistakes in that function that is giving you problems, so I will go through them.
Code:
int* ia = new int[1+rowsb];
int* ja = new int[1+rowsb];
double* ar = new double[1+rowsb];
double z;
double *x = new double[rowsobj];
double *p = new double[rowsf];
First, let's look at this above. This code causes a memory leak as you never clean up the memory. If you repeatedly call linprog1() in your main application, you will exhaust the heap of free memory. Nowhere is a delete[] issued for any of those calls to new[]. Then you have this in your solverr() function:
Code:
double** Barray = new double*[B.n_rows];
MATtoARR(B, Barray);
//...
double* farray = new double[f.size()];
VECtoARR(f, farray);
double* objarray = new double[OBJ.size()];
VECtoARR(OBJ, objarray);
Again, no call to delete[] to clean up the memory. If you were to call this function repeatedly throughout a program, the memory/resource leak adds up here.
In modern C++ programming, the use of proper container classes instead of the "new[]/delete[]" combination is what is done to implement dynamic arrays. Containers such as std::vector, or if using MFC, CArray, supersedes the usage of using new[]/delete[] in this way. Unless what is written is some sort of memory allocator or you're writing your own container class, new[]/delete[] should not be used if your goal is a dynamic array, unless you can justify its usage.
The reason why container classes are better than new[]/delete[]:
1) The memory allocation and deallocation is automatically done without you having to do anything yourself. This allows you to create the object that wraps a dynamic array, use it, and when the function or functional block exits, the object is destroyed via destructor. No need for you to call delete[] anywhere.
2) If your function returns for any reason, whether it is through a return statement, the end of the function is reached, or an exception is thrown causing the code to leave the function, the container class's destructor cleans up the memory. Using new[]/delete[], you need to stick a delete[] for every single return point in your program, and you also have to write a catch() block for any every exception that may be thrown, since you have to delete[] the memory inside the catch blocks (the latter case makes it almost a requirement to use a container class as opposed to new[]/delete[]).
So which sounds easier and safer? The new[]/delete[] or the container classes?
Another problem -- you're calling functions without caring if what is returned is valid. For example:
Code:
lp = glp_create_prob();
You then use "lp" assuming that is valid. How do you know that lp is not NULL or something that can't really be used?
Also, you should name your variables with descriptive names, not "x" and "p" as you've done in that function.
Last, are you debugging your code using the debugger? Did you unit test the function you wrote, or did you write the whole thing and expected it to work right out of the box?
#include <stdio.h>
#include <iostream>
#include <stdlib.h>
#include <glpk.h>
#include <armadillo>
#include <vector>
using namespace std;
using namespace arma;
int linprog1(const std::vector<double*>& Barray, int rowsb, const std::vector<double>& farray, int rowsf, const std::vector<double>& objarray, int rowsobj) // rowsb = B.n_rows,
{
glp_prob *lp;
std::vector<int> ia(1+rowsb);
std::vector<int> ja(1+rowsb);
std::vector<double> ar(1+rowsb);
double z;
std::vector<double> x(rowsobj);
std::vector<double> p(rowsf);
lp = glp_create_prob();
glp_set_prob_name(lp, "sample");
glp_add_rows(lp, rowsf);
for (int i = 0; i<rowsf; i++)
{
glp_set_row_bnds(lp, i+1, GLP_FX, farray[i], farray[i]);
}
glp_add_cols(lp, rowsobj);
for (int i = 0; i<rowsobj; i++)
{
glp_set_col_bnds(lp, i+1, GLP_LO, 0.0, 0.0);
glp_set_obj_coef(lp, i+1, objarray[i]);
}
for (int i = 0; i<rowsb; i++)
{
ia[i+1] = (int)Barray[i][0];
ja[i+1] = (int)Barray[i][1];
ar[i+1] = (int)Barray[i][2];
}
glp_load_matrix(lp, rowsobj*rowsf, &ia[0], &ja[0], &ar[0]);
glp_simplex(lp, NULL);
z = glp_get_obj_val(lp);
for (int i = 0; i<rowsobj; i++)
{
x[i] = glp_get_col_prim(lp, i+1);
}
glp_delete_prob(lp);
return 0;
}
Eliminating the comments and code that just does printing, the above does not cause a memory leak. Now it still doesn't address the issues I mentioned concerning not checking your return values from various functions.
You need to start using proper container classes such as vector instead of the new[] / delete[], then all or most of that pointer usage is not necessary at all and the program becomes safer and less prone to memory related bugs. As a matter of fact, I bet that even the linprog function parameters itself need not be pointers, but references to vectors (as my code shows). The only thing that needs a pointer are the third-party library functions, and I've demonstrated above how to use vector to "fool" those functions into using a vector as a parameter instead of a pointer (pass a pointer to the first element of the vector).
Also, check your return values, and debug your code step by step using the debugger.
Regards,
Paul McKenzie
Last edited by Paul McKenzie; December 8th, 2012 at 10:24 AM.
This is all very useful, and the std::Vector (or rather arma::Col) that I am now using thanks to you works great!
I actually tried to do it this way earlier, but did not add the [0] where I called the variables. Why is this zero necessary? What is it?
Thanks again, if you don't mind I may find myself uploading one or two more questions to this thread later on in the hope that you may be able to help me again
This is all very useful, and the std::Vector (or rather arma::Col) that I am now using thanks to you works great!
Not surprising. A lot of errors become magically eliminated once a vector is used -- the memory leaks clear up, and the hard-to-maintain pointers also become a moot point.
The only thing that can't be corrected by using vector is if you access an element out-of-bounds using the [] operator. For that, vector has the at() function, but that requires you to change your code to call at() instead of using [].
I actually tried to do it this way earlier, but did not add the [0] where I called the variables. Why is this zero necessary? What is it?
The vector has an internal array. To access the internal array, you need to get a pointer to the first element of the array. That's why you need to get the address of the first element. (The vector's [] is actually an overloaded operator that gets to the array element).
The CArray is the same thing as vector in terms of what they accomplish. All of that code you had now with new[]/delete[] is already done by CArray and vector internally (but with smarts added to it that you may not have even considered). So basically all you were doing is copying what vector/CArray was doing, but you were doing things the naive way while those container classes are much smarter and use new[]/delete[] and free-store properly, safely, and optimally.
I feel the code should make more sense now, but the "The program '[16172] top.exe: Native' has exited with code 3 (0x3)." error still appears... Here is the code now if you feel like having a look.
Code:
#include <stdio.h>
#include <iostream>
#include <stdlib.h>
#include <glpk.h>
#include <armadillo>
using namespace std;
using namespace arma;
void linprog1(arma::Mat<double> B, arma::Col<double> f, arma::Col<double> OBJ) // rowsb = B.n_rows (sp), linprog in matlab: coeff(obj), constraint coeff(B), right hand sides(f), lower bound(X) WHAT I WANT IN PI-SYMBOL LAGRANGE
{
glp_prob *lp;
arma::Col<int> ia(1+B.n_rows);
arma::Col<int> ja(1+B.n_rows);
arma::Col<double> ar(1+B.n_rows);
double z;
arma::Col<double> x(OBJ.n_rows);
arma::Col<double> pok(f.n_rows);
lp = glp_create_prob();
glp_set_prob_name(lp, "sample");
glp_add_rows(lp, f.n_rows);
for (int i = 0; i<f.n_rows; i++){
glp_set_row_bnds(lp, i+1, GLP_FX, f(i), f(i));
}
glp_add_cols(lp, OBJ.n_rows);
for (int i = 0; i<OBJ.n_rows; i++){
glp_set_col_bnds(lp, i+1, GLP_LO, 0.0, 0.0);
glp_set_obj_coef(lp, i+1, OBJ(i));
}
for (int i = 0; i<B.n_rows; i++){
ia(i+1) = B(i,0);
ja(i+1) = B(i,1);
ar(i+1) = B(i,2);
}
glp_load_matrix(lp, OBJ.n_rows*3, &ia(1), &ja(1), &ar(1));
glp_interior(lp, NULL);
z = glp_ipt_obj_val(lp);
for (int i = 0; i<f.n_rows; i++){
pok(i) = glp_ipt_row_dual(lp, i+1);
}
for (int i = 0; i<OBJ.n_rows; i++){
x(i) = glp_ipt_col_prim(lp, i+1);
}
glp_delete_prob(lp);
int cat;
cin >> cat;
return;
}
/* eof */
I keep trying to figure out at what point in the code the error appears, but I don't really know where to start with that.
You're passing these parameters by value. This is not optimal, or even correct.
You should be passing these by reference or const reference. There are only a few situations where an object must be passed by value, otherwise you should always pass by reference or const reference. Take a look more closely at my edited post above.
Passing by value is not optimal, as a copy of the data has to be made to the function. Also, passing these objects by value may be an error, especially if your class doesn't have a proper coded copy/assignment operator, and if you're expecting the function to retain the results in the passed-in objects.
Alright, that is very good to know, I thought the variables were only to be called in that way when you desired to modify their value within the function.
The function now runs, and the Linear Program that it is supposed to create runs fine. The only problem is that the output of the function doesn't make sense, which makes me think that I am making an error in my code somewhere, the code is
Code:
#include <iostream>
#include <stdlib.h>
#include <glpk.h>
#include <armadillo>
using namespace std;
using namespace arma;
void linprog1(arma::Mat<double>& A_sp, arma::Col<double>& b, arma::Col<double>& C) // rowsb = B.n_rows (sp), linprog in matlab: coeff(C), constraint coeff(B), right hand sides(f), lower bound(X) WHAT I WANT IN PI-SYMBOL LAGRANGE
{
int stopper;
glp_prob *lp;
arma::Col<int> ia(1+A_sp.n_rows);
arma::Col<int> ja(1+A_sp.n_rows);
arma::Col<double> ar(1+A_sp.n_rows);
double z;
arma::Col<double> x(C.n_rows);
arma::Col<double> pok(b.n_rows);
lp = glp_create_prob();
glp_set_prob_name(lp, "sample");
glp_add_rows(lp, b.n_rows);
for (int i = 0; i<b.n_rows; i++){
glp_set_row_bnds(lp, i+1, GLP_FX, b(i), b(i));
}
glp_add_cols(lp, C.n_rows);
for (int i = 0; i<C.n_rows; i++){
glp_set_col_bnds(lp, i+1, GLP_LO, 0.0, 0.0);
glp_set_obj_coef(lp, i+1, C(i));
}
for (int i = 0; i<A_sp.n_rows; i++){
ia(i+1) = A_sp(i,0) + 1;
ja(i+1) = A_sp(i,1) + 1;
ar(i+1) = A_sp(i,2);
cout << &ia(1) << " " << i+1 << endl;
}
//cin >> stopper;
glp_load_matrix(lp, A_sp.n_rows - 1, &ia(1), &ja(1), &ar(1));
glp_write_mps(lp, GLP_MPS_FILE, NULL, "herro.mps");
glp_interior(lp, NULL);
glp_write_mps(lp, GLP_MPS_FILE, NULL, "herro2.mps");
z = glp_ipt_obj_val(lp);
for (int i = 0; i<b.n_rows; i++){
pok(i) = glp_ipt_row_dual(lp, i+1);
}
for (int i = 0; i<C.n_rows; i++){
x(i) = glp_ipt_col_prim(lp, i+1);
}
glp_delete_prob(lp);
cin >> stopper;
return;
}
/* eof */
The variables sent to the function are read from row/column 1 rather than 0 which is why I have added ones in certain places, however I am wondering whether I am sending through a variable too many or too few in one place or another... The reason I think so is because of this line:
#include <iostream>
#include <stdlib.h>
#include <glpk.h>
#include <armadillo>
using namespace std;
using namespace arma;
void linprog1(arma::Mat<double>& A_sp, arma::Col<double>& b,
I have no idea what a "Col" or "Mat" is. These are proprietary types as they are not standard C++ types or MFC types.
however I am wondering whether I am sending through a variable too many or too few in one place or another....
...
Instead of wondering what may be happening, you need to debug your code using the debugger to find out where things go wrong. There is no need to be using cout statements to debug or random guesses and trial/error -- the Visual C++ compiler comes with one of the most powerful debuggers created for PC based C++ systems.
At this point, for time reasons, I have to leave the programming part of my dissertation for a few week.
I therefore wanted to take this opportunity to thank you for your help and patience. I have saved this conversation to my pc, and have learnt more from it than I have from any text-books.
Thanks for your help, and you'll see me back on this forum in a month or so!
Reply With Quote
Originally Posted by gugge444
Bookmarks