Skip to content

Commit

Permalink
fix: Benchmark duplicated metrics (#761)
Browse files Browse the repository at this point in the history
This pull request resolves a bug in the benchmark module where metrics
are duplicated when running it multiple times, leading to inconsistent
results.

The problem is caused by a combination of an existing benchmark config
that is created after the first run and the property
`inheritedSolverBenchmark`. Therefore, the method
`ai.timefold.solver.benchmark.impl.DefaultPlannerBenchmarkFactory#buildEffectiveSolverBenchmarkConfigList`
will duplicate the values when inheriting the configuration at the line
`#163`.

The proposed solution fixes the problem by avoiding the duplicates in
the config class `ProblemBenchmarksConfig`.
  • Loading branch information
zepfred authored Mar 28, 2024
1 parent 0a25027 commit c6f1e4f
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ public ProblemBenchmarksConfig inherit(ProblemBenchmarksConfig inheritedConfig)
inheritedConfig.getInputSolutionFileList());
problemStatisticEnabled = ConfigUtils.inheritOverwritableProperty(problemStatisticEnabled,
inheritedConfig.getProblemStatisticEnabled());
problemStatisticTypeList = ConfigUtils.inheritMergeableListProperty(problemStatisticTypeList,
problemStatisticTypeList = ConfigUtils.inheritUniqueMergeableListProperty(problemStatisticTypeList,
inheritedConfig.getProblemStatisticTypeList());
singleStatisticTypeList = ConfigUtils.inheritMergeableListProperty(singleStatisticTypeList,
singleStatisticTypeList = ConfigUtils.inheritUniqueMergeableListProperty(singleStatisticTypeList,
inheritedConfig.getSingleStatisticTypeList());
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,17 @@ void testDetermineSingleStatisticTypeList() {
assertThat(problemBenchmarksConfig.determineSingleStatisticTypeList())
.containsExactly(SingleStatisticType.CONSTRAINT_MATCH_TOTAL_STEP_SCORE);
}

@Test
void testDuplicatedDefaultMetrics() {
ProblemBenchmarksConfig benchmarksConfig = new ProblemBenchmarksConfig();
benchmarksConfig.withProblemStatisticTypeList(List.of(ProblemStatisticType.BEST_SCORE));
SolverBenchmarkConfig defaultConfig = new SolverBenchmarkConfig();
defaultConfig.withProblemBenchmarksConfig(benchmarksConfig);

SolverBenchmarkConfig inheritedConfig = new SolverBenchmarkConfig();
inheritedConfig.withProblemBenchmarksConfig(benchmarksConfig);
inheritedConfig.inherit(defaultConfig);
assertThat(inheritedConfig.getProblemBenchmarksConfig().getProblemStatisticTypeList()).hasSize(1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand Down Expand Up @@ -204,13 +206,26 @@ public static <T> List<T> inheritMergeableListProperty(List<T> originalList, Lis
}
}

public static <T> List<T> inheritUniqueMergeableListProperty(List<T> originalList, List<T> inheritedList) {
if (inheritedList == null) {
return originalList;
} else if (originalList == null) {
// Shallow clone due to modifications after calling inherit
return new ArrayList<>(inheritedList);
} else {
// The inheritedMap should be before the originalMap
Set<T> mergedSet = new LinkedHashSet<>(inheritedList);
mergedSet.addAll(originalList);
return new ArrayList<>(mergedSet);
}
}

public static <K, T> Map<K, T> inheritMergeableMapProperty(Map<K, T> originalMap, Map<K, T> inheritedMap) {
if (inheritedMap == null) {
return originalMap;
} else if (originalMap == null) {
return inheritedMap;
} else {
// The inheritedMap should be before the originalMap
Map<K, T> mergedMap = new LinkedHashMap<>(inheritedMap);
mergedMap.putAll(originalMap);
return mergedMap;
Expand Down

0 comments on commit c6f1e4f

Please sign in to comment.