StopFirstを実装した #808
StopFirstを実装した #808
Conversation
|
No problem. Good job! |
| final List<Variant> variantsByMutation = mutation.exec(variantStore); | ||
| final List<Variant> variantsByCrossover = crossover.exec(variantStore); | ||
| final List<Variant> variantsByMutation = mutation.exec(variantStore, | ||
| config.getRequiredSolutionsCount()); |
There was a problem hiding this comment.
reqCountは Mutatoin#exec に渡すよりも Mutation#new で渡した方が良いと思います．
・可変なパラメタではなく常に単一の値を取るのでコンストラクタが自然
・execで渡すよりもコンストラクタの方が変更波及が小さい
コンストラクタで設定されるmutationGeneratingCountと同じ性質だと思います．
public Mutation(final int mutationGeneratingCount, final Random random,
final CandidateSelection candidateSelection) {
this.random = random;
this.mutationGeneratingCount = mutationGeneratingCount;
this.candidateSelection = candidateSelection;
}| variantStore.addGeneratedVariants(variantsByMutation); | ||
| final List<Variant> variantsByCrossover = crossover.exec(variantStore, | ||
| config.getRequiredSolutionsCount()); |
|
|
||
| // 新しい修正プログラムが生成された場合，必要数に達しているかを調べる | ||
| // 達している場合はこれ以上の変異プログラムは生成しない | ||
| if (newVariant.isCompleted() && requiredSolutions <= ++foundSolutions) { |
There was a problem hiding this comment.
こちらのコードを推したいです．
複合条件if内でのインクリメント演算はバグの元，かつ混乱の元．
if (newVariant.isCompleted()) {
foundSolutions++;
}
if (foundSolutions >= requiredSolutions) {
break;
}| final List<Variant> variantList = simpleMutation.exec(variantStore, 1); | ||
|
|
||
| // 変異プログラムを全く生成しないはず | ||
| assertThat(variantList).hasSize(0); |
There was a problem hiding this comment.
assertThat(variantList).isEmpty();| break VARIANT_GENERATION; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
GOTOを避けたいのでメソッドに切り出した方がよさそう
final List<Variant> variants = generateVariants(variantStore, validVariants);
// バリアントを作りすぎた場合はそれを除いてリターン
return generatingCount < variants.size() ? variants.subList(0, generatingCount) : variants;
}
private List<Variant> generateVariants(final VariantStore variantStore,
final List<Variant> validVariants) {
final List<Variant> generatedVariants = new ArrayList<>();
int foundSolutions = variantStore.getFoundSolutionsNumber()
.get();
// generatingCountを超えるまでバリアントを作りづづける
while (generatedVariants.size() < generatingCount) {
try {
generatedVariants.addAll(makeVariants(validVariants, variantStore));
foundSolutions += generatedVariants.stream()
.filter(Variant::isCompleted)
.count();
if (requiredSolutions <= foundSolutions) {
return generatedVariants;
}
} catch (final CrossoverInfeasibleException e) {
log.debug(e.getMessage());
}
}
return generatedVariants;
}厳密には元のコードと挙動は違います．
そのため RandomCrossoverTest#testStopFirst02 が落ちます．
詳細は後述
| final List<Variant> variants = crossover.exec(testVariants.variantStore, 1); | ||
|
|
||
| // 交叉で1つだけ変異プログラムを生成するはず | ||
| assertThat(variants).hasSize(1); |
There was a problem hiding this comment.
先のテストが落ちる問題の箇所です．
stopFirst=onにすると解発見時に強制的にGAを止める，という点は認識が一致してると思います．
ただ，その際に以下A/Bどちらの振る舞いをするかの認識違いがありそう．
A. 常に「発見した解の数<=期待する解の数」を満たす．
B. 常に「発見した解の数<=期待する解の数」を満たすとは限らない．
Mutationは一つずつ個体を生成するのでAのような挙動でも自然です．
一方，CrossOverは複数個体をまとめて生成します．
当然その中には解個体が複数含まれます．
その際に，せっかく見つけた（コンパイル+評価済みの）解個体を捨ててAを満たすよりは，
Bのように返した方が損がないと思います．
あくまで「stopFirstはGAのプロセスを強制停止して時間を節約する機能」であり，
Aを満たすか否かは別の問題だと思います．
There was a problem hiding this comment.
個体の実態はLazyVariantなので，isCompletedが呼び出されたときにテスト実行がされますよね．
Bだと交叉により生成した全ての個体に対してisCompletedが呼び出されます（テストが実行される）．
なので，stopFirstにする意義が薄れるように思いますがどうでしょうか？
There was a problem hiding this comment.
遅延評価を忘れてました．
100%agreeです．
以下のstream使ってさらっとやってる部分を手続き的にやるしかないですね．
とりあえず，return使えばgotoは消えると思います．
while (generatedVariants.size() < generatingCount) {
try {
generatedVariants.addAll(makeVariants(validVariants, variantStore));
foundSolutions += generatedVariants.stream()
.filter(Variant::isCompleted)
.count();
if (requiredSolutions <= foundSolutions) {
return generatedVariants;
}
} catch (final CrossoverInfeasibleException e) {
log.debug(e.getMessage());
}
}There was a problem hiding this comment.
レビューありがとうございました．
修正したのでもう一度確認お願いします．
| when(testVariants.variantStore.createVariant(any(), any())).then(ans -> { | ||
| return new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null, | ||
| ans.getArgument(1)); | ||
| }); |
There was a problem hiding this comment.
return 省略できます．
when(testVariants.variantStore.createVariant(any(), any())).then(ans ->
new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null,
ans.getArgument(1))
);| when(testVariants.variantStore.createVariant(any(), any())).then(ans -> { | ||
| return new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null, | ||
| ans.getArgument(1)); | ||
| }); |
| when(testVariants.variantStore.createVariant(any(), any())).then(ans -> { | ||
| return new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null, | ||
| ans.getArgument(1)); | ||
| }); |
| when(variantStore.createVariant(any(), any())).then(ans -> { | ||
| return new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null, | ||
| ans.getArgument(1)); | ||
| }); |
| // 修正プログラムが必ず生成されるようにモックを設定する | ||
| when(testVariants.variantStore.createVariant(any(), any())).then(ans -> { | ||
| return new Variant(0, 0, ans.getArgument(0), null, null, new SimpleFitness(1.0), null, | ||
| ans.getArgument(1)); | ||
| }); |
|
修正しましたので，再度確認お願いします． |
resolve #434
ある世代Nにおいて，必要な数の修正プログラムを生成した場合の挙動を変更した．
旧：その世代の最後まで引き続き変異プログラムの生成とテスト実行の処理を行う
新：ただちにその世代の処理を終える