1

I'm working on a GA and seem to be having problems with the tournament selection. I think this is due to the fact that I'm not comparing what I want to compare (in terms of fitness values)

srand(static_cast <unsigned> (time(0)));
    population Pop;
    vector<population> popvector;
    vector<population> survivors;

    population *ptrP;

    for (int i = 0; i <= 102; i++)
    {
        ptrP = new population;
        ptrP->generatefit;
        ptrP->findfit;
        popvector.push_back(*ptrP);
        //include finding the persons "overall".  WIP

    }
    cout << "The fit values of the population are listed here: " << endl;
    vector<population> ::iterator it; //iterator to print everything in the vector 
    for (it = popvector.begin(); it != popvector.end(); ++it)
    {
        it->printinfo();
    }

    unsigned seed = std::chrono::system_clock::now().time_since_epoch().count(); // generate a seed for the shuffle process of the vector.

    cout << "Beggining selection process" << endl;

     shuffle(popvector.begin(), popvector.end(), std::default_random_engine(seed));

    //Shuffling done to randomize the parents I will be taking.     
    // I also want want to pick consecutive parents 
    for (int i = 0; i <= 102; i = i + 3)
    {
        if (popvector[i] >= popvector[i++]);

    }


}

Now what I think my problem is, is that when im trying to compare the Overall values (Not found yet, working on how to properly model them to give me accurate Overall fitness values) I'm not comparing what I should be.

I'm thinking that once I find the persons "Overall" I should store it in a Float vector and proceed from there, but I'm unsure if this is the right way to proceed if I wish to create a new "parent" pool, since (I think) the "parent pool" has to be part of my population class.

Any feedback is appreciated.

Ploxzx
  • 53
  • 3
  • Besides the issue you're having, you also have a horrible memory leak. You're `new`-ing a `population` on the heap, then copying into a vector, and then failing to `delete` what you `new`. Rule of thumb: if you `new` something, somewhere, at some point you have to `delete` it. – Sam Varshavchik Apr 07 '16 at 02:18
  • @SamVarshavchik This whole code is currently a WIP, haven't really gotten around to any of the details yet. I just want to make sure my functions run before I fine tune the code. That being said, where would you delete the population? At the end of everything or at the end of each loop? – Ploxzx Apr 07 '16 at 02:26
  • Don't allocate it in the first place. Just use `population p; p.generatefit(); p.findfit(); popvector.emplace_back( std::move( p ) );`. Now, notice I've added parentheses to your function calls. Perhaps that was your intent? If you don't do that, it's just a no-op and your function is not called. Maybe that's part of the problem. – paddy Apr 07 '16 at 02:34
  • I can't believe I didn't add () to my function calls. I have no idea why it alluded me. I feel as if I don't need to use a pointer but I wanted to have one just in case I need to use one later on for something. For now I'll remove it and if I need it at some point I'll just add it. @paddy – Ploxzx Apr 07 '16 at 02:42
  • That's fine, but if you want to use dynamic memory, make sure you actually store the pointer in your vector. The way you do it now, you are still storing a value which is copied from the other one you allocated, and then the pointer is lost. So to use a pointer, you would have the type: `std::vector`. Or even better: `std::vector>`. – paddy Apr 07 '16 at 02:44
  • @[addy I was unfamiliar with that syntax. If I dynamically allocate it, I will be sure to follow it. Also, where would you delete the allocated memory? – Ploxzx Apr 07 '16 at 03:07

1 Answers1

1

srand(static_cast <unsigned> (time(0)));

This is useless: you're calling std::shuffle in a form not based on std::rand:

shuffle(popvector.begin(), popvector.end(), std::default_random_engine(seed));

If somewhere else in the program you need to generate random numbers, do it via functions / distributions / engines in random pseudo-random number generation library (do not use std::rand).

Also consider that, for debugging purpose, you should have a way to initialize the random engine with a fixed seed (debug needs repeatable results).

for (int i = 0; i <= 102; i++)

Do not use magic numbers.

Why 102? If it's the population size, store it in a constant / variable (populationSize?), document the variable use and "enjoy" the fact that when you need to change the value you haven't to remember the locations where it's used (just in this simple snippet there are two distinct use points).

Also consider that the population size is one of those parameters you need to change quite often in GA.

ptrP = new population;
ptrP->generatefit;
ptrP->findfit;
popvector.push_back(*ptrP);

Absolutely consider Sam Varshavchik's and paddy's remarks.

for (int i = 0; i <= 102; i = i + 3)
{
  if (popvector[i] >= popvector[i++]);
  // ...

Generally it's not a good practice to change the index variable inside the body of a for loop (in some languages, not C / C++, the loop variable is immutable within the scope of the loop body).

Here you also have an undefined behaviour:

popvector[i] >= popvector[i++]

is equivalent to

operator>=(popvector[i], popvector[i++])

The order that function parameters are evaluated is unspecified. So you may have:

auto a = popvector[i];
auto b = popvector[i++];
operator>=(a, b);  // i.e. popvector[i] >= popvector[i]

or

auto b = popvector[i++];
auto a = popvector[i];
operator>=(a, b);   // i.e. popvector[i + 1] >= popvector[i]

Both cases are wrong.

In the first case you're comparing the same elements and the expression is always true.

In the second case the comparison probably is the opposite of what you were thinking.

Take a look at:

and always compile source code with -Wall -Wextra (or their equivalent).

I'm not sure to correctly understand the role of the class population. It may be that the name is misleading.

Other questions / answers you could find interesting:

Community
  • 1
  • 1
manlio
  • 18,345
  • 14
  • 76
  • 126