onclave / NSGA-II

an implementation of NSGA-II in java
MIT License
44 stars 23 forks source link

Question about Chromosome.reset() #12

Closed NordelDev closed 3 years ago

NordelDev commented 3 years ago

Hi,

I checked your code and got the following question:

In your class [Chromosome](https://github.com/onclave/NSGA-II/blob/master/src/main/java/com/debacharya/nsgaii/datastructure/Chromosome.java) you instantiate an object with the Rank = -1.

But in your method reset() you set the rank to Integer.MAX_VALUE.

Is there a specific reason for that?

Because when I have a look at the call of reset() in class [NSGA2.java](https://github.com/onclave/NSGA-II/blob/master/src/main/java/com/debacharya/nsgaii/NSGA2.java) this seems to be "wrong" because there you increase the rank value by one to the already max Integer value.

Integer.MAX_VALUE = 2147483647
Integer.MAX_VALUE + 1 = -2147483648

In the end this new value has still the lowest rank value but isn't it easier and more intuitive to reset the rank to -1 or 0? (also for readability)

onclave commented 3 years ago

Okay, this is a nice catch. Thank you for reporting this. But I'll have to take a deeper look at this. I have a logic behind this, but it might be flawed based on what you are pointing out. Let me check, run some tests and get back to you. For the time being, I'll mark your issue as a bug report.

You have actually pointed out a probable bug in another part of my code instead. The real problem is not that the rank is marked -1 in one place and Integer.MAX_VALUE at another. I have a logic behind this. But your insight has pointed out a potential bug in another part of the code. Let me check, and I'll explain everything.

Again, thank you, for reporting this!

onclave commented 3 years ago

@DubWa91 I ran a few test and resolved the issue. There was a bug. I have pushed a new commit as well with the update. I have also released a new version on maven at 3.2.0. Now, let me explain what was wrong.

Firstly, yes, what you have reported can be considered a bug where in one place, the chromosomes had an initial rank of -1 whereas during reset, they were given Integer.MAX_VALUE. But this did not create an issue because the Fast Non-Dominated Sort in NSGA2.java was making the actual mistake of not using this initial rank value as a check to which it was intended for, and thanks to you reporting this, I came to realise that this initial rank value was never used anywhere and thus I spotted a bug.

You see, during fast non-dominated sorting, when the first rank 1 chromosomes have been marked, the next steps were as follows:

step 01: check if there are unranked chromosomes (rank = -1), if yes, go to step 02, otherwise stop step 02: for every chromosome in the populace, go to step 03 step 03: if the chromosome is not ranked (rank = -1), go to step 04, otherwise go to step 02 step 04: for every dominated chromosome of the selected chromosome from step 02, go to step 05 step 05: if dominated chromosome's dominated count > 0, go to step 06, otherwise go to step 04 step 06: decrement dominated count of dominated chromosome by 1 step 07: if dominated count of dominated chromosome == 0, go to step 08, otherwise go to step 04 step 08: set rank of dominated chromosome to (rank of chromosome from step 02 + 1). Go to step 01

Otherwise, (previously), some of the chromosomes might stay at rank -2147483648 as you indicated since it was not checking whether the parent chromosome was actually ranked or not.

The issue has been fixed now, and you can check the new code. Once again, thank you for pointing out this bug.

NordelDev commented 3 years ago

Thank you very much for the notification to your update :)

Also also thank you for the detailed explanation.

Gladto hear, that my information lead to a bug fix ;)