giacomelli / GeneticSharp

GeneticSharp is a fast, extensible, multi-platform and multithreading C# Genetic Algorithm library that simplifies the development of applications using Genetic Algorithms (GAs).
MIT License
1.27k stars 332 forks source link

RouletteWheelSelection.SelectFromWheel throws NullReferenceException when first generation is "invalid" #57

Closed codingdna2 closed 5 years ago

codingdna2 commented 5 years ago

To Reproduce RouletteWheelSelection.SelectFromWheel throws NullReferenceException when first generation is "invalid" (all chromosome have zero fitness).

Expected behavior A better exception should be thrown. NullReferenceException is confusing and require to debug GeneticSharp.

Version: 2.2.0

Fix Proposal (probably naive)

Let me know if you like it and if you want me to make a PR.

protected static IList<IChromosome> SelectFromWheel(int number, IList<IChromosome> chromosomes, IList<double> rouletteWheel, Func<double> getPointer)
{
    var selected = new List<IChromosome>();

    for (int i = 0; i < number; i++)
    {
        var pointer = getPointer();

        var chromosomeIndex = rouletteWheel.Select((value, index) => new { Value = value, Index = index }).FirstOrDefault(r => r.Value >= pointer)?.Index;
        if (chromosomeIndex == null)
            throw new SelectionException("Generation doesn't contains any chromosome with fitness greater than 0");

        selected.Add(chromosomes[chromosomeIndex.Value]);
    }

    return selected;
}
giacomelli commented 5 years ago

Yeah, please make the PR, but add unit test method to validate this case.

giacomelli commented 5 years ago

Looking better you can use the method that already on ChromosomeExtensions: void ValidateGenes(this IList<IChromosome> chromosomes) to validate all genes outside the for loop or you can use the method void ValidateGenes(this IChromosome chromosome) to something similar what you purposed.

codingdna2 commented 5 years ago

It's not clear the last hint you gave me. In order to include it in ValidateGenes, I shall include something like:

if (chromosomes.All(c => c.Fitness == 0))
{
    throw new InvalidOperationException("Generation doesn't contains any chromosome with fitness greater than 0");
}

But I'm not sure this kind of check in ValidateGenes can have side effects.. Is this what you meant?

giacomelli commented 5 years ago

Yeah, you're right, the ChromosomeExtensions' methods validate other things, but I'm no sure about validate c.Fitness == 0, because some kind o problems can have Fitness zero (in some cases negative fitness, so zero is the best one).

Could you please write a unit test that exposes this problem, then I'll have a better understand and solution suggestion for it.

codingdna2 commented 5 years ago

Unit Test:

   [Test()]
    public void SelectChromosomes_Generation_ChromosomesZeroFitness()
    {
        var target = new RouletteWheelSelection();
        var c1 = Substitute.ForPartsOf<ChromosomeBase>(2);
        c1.Fitness = 0;

        var c2 = Substitute.ForPartsOf<ChromosomeBase>(2);
        c2.Fitness = 0;

        var c3 = Substitute.ForPartsOf<ChromosomeBase>(2);
        c3.Fitness = 0;

        var c4 = Substitute.ForPartsOf<ChromosomeBase>(2);
        c4.Fitness = 0;

        var generation = new Generation(1, new List<IChromosome>() {
            c1, c2, c3, c4
        });

        var actual = target.SelectChromosomes(2, generation);
    }