New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented the Adaptive Epsilon Sampling EA #7
Conversation
…ards different parent and offspring numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, we should also move the huge number of specific AeSeH classes to another subpackage ea.aeseh
build.gradle
Outdated
@@ -1,5 +1,6 @@ | |||
plugins { | |||
id 'com.github.kt3k.coveralls' version '2.6.3' | |||
id "org.sonarqube" version "2.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working? Should be another feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works for the sonarqube I am running on my local machine. I oversaw that it was still in the build.gradle when merging the branches. I normally to not commit this change because I only use it locally.
build.gradle
Outdated
@@ -21,6 +22,7 @@ allprojects { | |||
group = 'org.opt4j' | |||
|
|||
repositories { | |||
jcenter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of Mockito. It is really cool for mocking up stuff. And it enable actual test-driven development where you can test the high-level classes without touching or even having an implementation of the low-level modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Then leave it in. Is mockito not available on maven central?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check. To be honest I just used the gradle dependency that was in the Mockito tutorial
opt4j-optimizers/build.gradle
Outdated
@@ -1,4 +1,7 @@ | |||
dependencies { | |||
compile project(':opt4j-core') | |||
compile project(':opt4j-operators') | |||
|
|||
testCompile group: 'junit', name: 'junit', version: '[4.0,)' | |||
testCompile "org.mockito:mockito-core:2.+" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also use scheme: group: '', name: '', version ''
do use a specific version of mockito (gradle eclipse often has some problems otherwise)
* Heidelberg, 2013. | ||
* | ||
* Please consider citing the paper if you use this class for a scientific | ||
* publication. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we propably should use this sentence either everywhere or nowhere.
You could also additionally use @Info annotation
Maybe (as another feature), we could create an @citation annotation to a) hold such information and b) print all citations which are used during an optimizuation.
* @author Fedor Smirnov | ||
* | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
* value where the number of neighborhoods is near a user-defined value. | ||
* | ||
* @param tooManyNeighborhoods | ||
* : TRUE => too many neighborhoods created; FALSE => not enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -47,19 +46,19 @@ | |||
protected int generations = 1000; | |||
|
|||
@Constant(value = "alpha", namespace = EvolutionaryAlgorithm.class) | |||
@Info("The size of the population.") | |||
@Info("Alpha - The size of the population.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
@Info("The size of the population α.")
@Info("The number of parents per generation μ.")
@Info("The number of offsprings per generation λ.")
work?
import org.opt4j.core.Objectives; | ||
|
||
/** | ||
* Class used for the non-dominated sorting. During non-dominated sorting, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: do not use "Class" but "The NonDominatedSorting sorts individuals into fronts." or similar.
*/ | ||
public class NonDominatedSorting { | ||
|
||
private NonDominatedSorting() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete unused constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a thing I got from SonarQube. It says that it is good practice to have a private constructor in classes that only offer static methods, so that no instance of the class can be created.
* @author Fedor Smirnov | ||
* | ||
*/ | ||
public class NonDominatedSorting { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
public class NonDominatedSorting extends ArrayList<List>
instead of all static methods, directly holding the front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to think about that
I totally agree with everything I did not reply to. Will implement the adjustments in the near future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor code style stuff.
} | ||
|
||
/** | ||
* sort the given individuals into non-dominated fronts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Comment should start with upper case
- Use third person
- Missing "." at the end of the sentence
- {@link Individual}s
See http://www.oracle.com/technetwork/articles/java/index-137868.html
--> Sorts the given {@link Individual}s into non-dominated fronts.
/** | ||
* sort the given individuals into non-dominated fronts | ||
* | ||
* @param individuals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add basic description http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#@param
*/ | ||
public void generateFronts(Collection<Individual> individuals) { | ||
// assign an id to each individual that corresponds to its index in an | ||
// array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start also code comments with upper case in case they are full sentences, end with "."
// Initialize a map where an individual is assigned to the individuals | ||
// that it dominates | ||
Map<Individual, List<Individual>> dominatedIndividualsMap = new HashMap<Individual, List<Individual>>(); | ||
// n is an array where for each individual, the number of individuals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is n?
import org.opt4j.core.Objectives; | ||
|
||
/** | ||
* The NonDominatedFronts sorts each evaluated individual into fronts based on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please link used classes in javadoc:
The {@link NonDominatedFronts} sorts each evaluated {@link Individual}...
public class AdditiveEpsilonMapping implements EpsilonMapping { | ||
|
||
@Override | ||
public Objectives mapObjectives(Objectives original, double epsilon, Map<Objective, Double> objectiveAmplitudes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc comment
} | ||
|
||
@Override | ||
public Map<Objective, Double> findObjectiveAmplitudes(Set<Individual> individuals) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I think that the javadocs generated for the interface of the class do provide an exact description of what the method does so that no additional comments are necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, sorry.
protected final int plannedNeighborhoodNumber; | ||
|
||
@Inject | ||
public AeSeHCoupler(EpsilonMapping epsilonMapping, EpsilonAdaption epsilonAdaption, Random random, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc comment
} | ||
|
||
@Override | ||
public Collection<Pair<Individual>> getCouples(int size, List<Individual> parents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc comment
protected final ESamplingSurvivorGeneration survivorGeneration; | ||
|
||
@Inject | ||
public AeSeHSelector(ESamplingSurvivorGeneration survivorGeneration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc comment
Okay, I made the javadoc comment adjustments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last remarks, I hope. Ideally, this process should not take so long...
/** | ||
* Package for the classes of the Adaptive ε-Sampling ε-Hood MOEA. | ||
*/ | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*/ /**
should just be one comment.
See for example org.opt4j.core.package-info.java. Some general information, how all the different classes/interfaces in this package interact would be great to easier get an overview.
@Override | ||
protected void config() { | ||
bindIterativeOptimizer(EvolutionaryAlgorithm.class); | ||
bind(CrossoverRate.class).to(ConstantCrossoverRate.class).in(SINGLETON); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we would remove line 256 and 257 we would have only one module for the general EA and this one for the aeseh selector and coupler.
Pro:
- This one module is a lot simpler without alpha, mu, and lambda.
- It would resemble the way we do it with Couplers in BasicMating and the Selectors NSGA, SMS, and SPEA
- You can freely combine selectors and couplers
Contra: - You would need to select two modules, the standard EA and the AeSeh. We could, however, document this.
* | ||
*/ | ||
@ImplementedBy(DefaultSurvivorGeneration.class) | ||
public interface ESamplingSurvivorGeneration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it probably that we get another Survivor Generation? Otherwise, we could remove the interface in favor for just a single class.
If not, I would nonetheless vote for renaming this interface to SurvivorGeneration and the DefaultSurvivorGeneration to EpsilonSamplingSurvivorGeneration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* | ||
*/ | ||
@ImplementedBy(AdditiveEpsilonMapping.class) | ||
public interface EpsilonMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it likely that we get another EpsilonMapping? To me, this also looks extremely AeSeH specific. If not, we could remove the interface in favor for just a single class.
} | ||
|
||
@Override | ||
public Map<Objective, Double> findObjectiveAmplitudes(Set<Individual> individuals) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, sorry.
* | ||
*/ | ||
@ImplementedBy(DefaultEpsilonAdaptation.class) | ||
public interface EpsilonAdaption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it likely that we get another EpsilonAdaption? To me, this also looks extremely AeSeH specific. If not, we could remove the interface in favor for just a single class.
@FedorSmirnov89 could you check and resolve the minor comments I added and also resolve the codacy problem? Then, we can merge this one. |
@felixreimann Okay, I will take care of it before the end of this week. |
Summary of the changes to the pull request:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about even more restructure this:
- Rename the Selector to EpsilonSamplingSelector and create a specific SelectorModule for it, according the other selectors: Elitism, Nsga2, Spea2, SMS
- Rename the Coupler to EpsilonNeighborhoodCoupler and add it to the BasicMatingModule (and perhaps rename it), according the other couplers: Random, unique, default
Then, the user could freely mix all the couplers and selectors.
Additionally, the AeSeHModule would just statically configure the eamodule, the EpsilonSamplingSelector, and the EpsilonNeighborhoodCoupler with exactly the values proposed in the paper such that the user could just select it if the exact aeseh implementation is required.
build.gradle
Outdated
@@ -1,5 +1,6 @@ | |||
plugins { | |||
id 'com.github.kt3k.coveralls' version '2.6.3' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
* {@link org.opt4j.optimizers.ea.aeseh.AeSeHModule} and the | ||
* {@link org.opt4j.optimizers.ea.EvolutionaryAlgorithmModule}. | ||
*/ | ||
package org.opt4j.optimizers.ea.aeseh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add \n
* In the Opt4J GUI, the AeSeH is configured using the | ||
* {@link org.opt4j.optimizers.ea.aeseh.AeSeHModule} and the | ||
* {@link org.opt4j.optimizers.ea.EvolutionaryAlgorithmModule}. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just an extremly long and awkward author name. Test javadoc generation.
Add <p>
as paragraph separator. See other javadocs.
*/ | ||
@Info("Multi-objective evolutionary algorithm where the survival selection and the creation of neighborhoods is based on epsilon-dominance. The selection of parents is done within the created neighborhoods.") | ||
@Citation(authors = "Hernán Aguirre, Akira Oyama, and Kiyoshi Tanaka", title = "Adaptive ε-sampling and ε-hood for evolutionary many-objective optimization.", journal = "Evolutionary Multi-Criterion Optimization (EMO)", pageFirst = 322, pageLast = 336, year = 2013, month = UNKNOWN) | ||
public class AeSeHModule extends OptimizerModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest that AeSeH should be an EA submodule as it is mainly an EA with a specific selector and coupler. Please add:
@Parent(EvolutionaryAlgorithmModule.class)
I agree that having the selectors and couplers as modules that can be implemented individually is a good idea. I have made changes to the coupler and selector of the AeSeH accordingly. However, for the other selectors and couplers, the treatment was somewhat inconsistent. While each selector can be added as a single module, the couplers are configured in the GUI using the BasicMatingModule. I would prefer a solution where each coupler is added as a module as well. I have implemented the modules and would propose to delete the BasicMatingModule. |
* the {@link AdaptiveEpsilon} contains the ε-value and the | ||
* information about its adaptation | ||
* @param epsilonTooBig | ||
* {@code} if the current epsilon is too big |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{@code what?} fix javadoc
* @param tooManyEpsilonDominantIndividuals | ||
* {@code true} if too many individuals have been created | ||
*/ | ||
//public void adaptSamplingEpsilon(boolean tooManyEpsilonDominantIndividuals); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete if unnecessary.
* @param tooManyNeighborhoods | ||
* {@code true} if too many individuals have been created | ||
*/ | ||
//public void adaptNeighborhoodEpsilon(boolean tooManyNeighborhoods); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete if unnecessary.
build.gradle
Outdated
@@ -1,5 +1,5 @@ | |||
plugins { | |||
id 'com.github.kt3k.coveralls' version '2.6.3' | |||
id 'com.github.kt3k.coveralls' version '2.6.3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let files unchanged if the changes have nothing to do with the main scope of the pull request.
* @author Fedor Smirnov | ||
* | ||
*/ | ||
public class NonDominatedFronts extends ArrayList<List<Individual>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think, this class should expose that it technically uses an arraylist<list> to the outside. Aren't there a lot of arraylist functions (or even all of them?), which make no sense on an object called "NonDominatedFronts"? Isn't this class more an algorithm instead of a data structure?
As I do not have good proposal how to restructure this one, this is not a blocker for merging. Just something I would ask you to rethink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The user should not be able to manipulate the list. I changed the class accordingly
import org.opt4j.optimizers.ea.Selector; | ||
|
||
/** | ||
* Module to bind the AeSeH evolutionary algorithm as optimizer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to docu: This module sets the parameters of {@link AeSehCoupler} and {@link AeSehSelector} as given in the paper. The corresponding {@link AeSehCouplerModule} and {@link AeSehSelectorModule} are then not required to be selected.
or something similar.
* | ||
*/ | ||
@Name("Epsilon Sampling") | ||
public class AeSeHSelectorModule extends SelectorModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @citation
* | ||
*/ | ||
@Name("Epsilon Sampling") | ||
public class AeSeHSelectorModule extends SelectorModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to EpsilonSamplingSelectorModule
and EpsilonSamplingSelector
, resp.
* @author Fedor Smirnov | ||
* | ||
*/ | ||
public class EpsilonAdaptationDefault implements EpsilonAdaptation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn`t this one such specific that the separation of class and interface is superfluous? If you can think about an alternative implementation, what is then specific about this one? -> If you have an answer, use it in the class name instead of "Default" and also in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class handles the adaptation of the epsilon throughout the exploration. At least for me, the exact way to adapt the epsilon would be one of the first things to play around with in an exploration. The implementation here realizes the adaptation presented in the paper. However, the authors do not provide any argument as to why this adaptation makes sense, so I definitely foresee different adaptation mechanisms in the future.
As for the name: What they are doing is adding/substracting a delta value which itself is then doubled/divided by 2. I find it hard to find a descriptive title here. For now, I went with EpsilonAdaptationDelta to signify that the adapted part is the delta value and not the epsilon. Not super happy with this name though. Any suggestions?
* {@link org.opt4j.optimizers.ea.EvolutionaryAlgorithmModule}. | ||
* </p> | ||
*/ | ||
package org.opt4j.optimizers.ea.aeseh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you tell me which newline you are referring to? The generated Javadoc seems fine to me.
seems to be nothing more to say about it
Implemented the Adaptive Epsilon Sampling EA proposed in the paper:
Aguirre, Hernán, Akira Oyama, and Kiyoshi Tanaka. "Adaptive ε-sampling and ε-hood for evolutionary many-objective optimization." International Conference on Evolutionary Multi-Criterion Optimization. Springer, Berlin, Heidelberg, 2013.
In its current state, it should perform well on problems with a high number (4 +) of objectives (experiments running). There is also an extension to improve the performance in cases with fewer objectives. I would like to integrate it into OPT4J even if it should be outperformed by NSGA2 because it has a more sophisticated algorithm for the survivor and parent selection and is, in my opinion, more suitable to try out new exploration approaches.