-
Notifications
You must be signed in to change notification settings - Fork 206
fix: seperate move and solver RandomGenerator usage #2356
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ public abstract class AbstractSolver<Solution_> implements Solver<Solution_> { | |
| protected final UniversalTermination<Solution_> globalTermination; | ||
| protected final List<Phase<Solution_>> phaseList; | ||
|
|
||
| private RandomGenerator.@Nullable SplittableGenerator savedRandom; | ||
| private RandomGenerator.@Nullable SplittableGenerator savedMoveGeneratorRandom; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does solver save the move random, and not the solver random? This is confusing; at the very least we have a naming problem here. |
||
|
|
||
| // ************************************************************************ | ||
| // Constructors and simple getters/setters | ||
|
|
@@ -131,16 +131,16 @@ public void stepStarted(AbstractStepScope<Solution_> stepScope) { | |
| globalTermination.stepStarted(stepScope); | ||
| // To ensure reproducibility even when the number of random calls is not deterministic, | ||
| // split the random at step start. | ||
| var delegatingRandom = ((DelegatingSplittableRandomGenerator) stepScope.getWorkingRandom()); | ||
| savedRandom = delegatingRandom.getDelegate(); | ||
| var delegatingRandom = ((DelegatingSplittableRandomGenerator) stepScope.getMoveGeneratorWorkingRandom()); | ||
| savedMoveGeneratorRandom = delegatingRandom.getDelegate(); | ||
| delegatingRandom.setDelegate(delegatingRandom.split()); | ||
| // Do not propagate to phases; the active phase does that for itself and they should not propagate further. | ||
| } | ||
|
|
||
| public void stepEnded(AbstractStepScope<Solution_> stepScope) { | ||
| // Restore from the split random | ||
| var delegatingRandom = ((DelegatingSplittableRandomGenerator) stepScope.getWorkingRandom()); | ||
| delegatingRandom.setDelegate(Objects.requireNonNull(savedRandom)); | ||
| var delegatingRandom = ((DelegatingSplittableRandomGenerator) stepScope.getMoveGeneratorWorkingRandom()); | ||
| delegatingRandom.setDelegate(Objects.requireNonNull(savedMoveGeneratorRandom)); | ||
| bestSolutionRecaller.stepEnded(stepScope); | ||
| phaseLifecycleSupport.fireStepEnded(stepScope); | ||
| globalTermination.stepEnded(stepScope); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,8 @@ public class SolverScope<Solution_> { | |
| private Set<SolverMetric> solverMetricSet = Collections.emptySet(); | ||
| private Tags monitoringTags; | ||
| private int startingSolverCount; | ||
| private RandomGenerator workingRandom; | ||
| private RandomGenerator moveGeneratorWorkingRandom; | ||
| private RandomGenerator solverWorkingRandom; | ||
| private InnerScoreDirector<Solution_, ?> scoreDirector; | ||
| private AbstractSolver<Solution_> solver; | ||
| private DefaultProblemChangeDirector<Solution_> problemChangeDirector; | ||
|
|
@@ -143,12 +144,25 @@ public void setStartingSolverCount(int startingSolverCount) { | |
| this.startingSolverCount = startingSolverCount; | ||
| } | ||
|
|
||
| public RandomGenerator getWorkingRandom() { | ||
| return workingRandom; | ||
| public RandomGenerator getMoveGeneratorWorkingRandom() { | ||
| return moveGeneratorWorkingRandom; | ||
| } | ||
|
|
||
| public void setWorkingRandom(RandomGenerator workingRandom) { | ||
| this.workingRandom = workingRandom; | ||
| public RandomGenerator getSolverWorkingRandom() { | ||
| return solverWorkingRandom; | ||
| } | ||
|
Comment on lines
+147
to
+153
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two need to get very good Javadocs. The whole "using extra random calls affects multi-threading" is not obvious at all. It needs to be clearly explained. |
||
|
|
||
| /** | ||
| * Only used for tests | ||
| */ | ||
| public void setTestWorkingRandom(RandomGenerator workingRandom) { | ||
| this.moveGeneratorWorkingRandom = workingRandom; | ||
| this.solverWorkingRandom = workingRandom; | ||
| } | ||
|
Comment on lines
+155
to
+161
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't. Find another way. Public methods on types as prominent as this must not be only used for testing. |
||
|
|
||
| public void setWorkingRandom(DelegatingSplittableRandomGenerator workingRandom) { | ||
| this.moveGeneratorWorkingRandom = workingRandom; | ||
| this.solverWorkingRandom = workingRandom.split(); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
|
|
@@ -354,9 +368,10 @@ public SolverScope<Solution_> createChildThreadSolverScope(ChildThreadType child | |
| childThreadSolverScope.solverMetricSet = solverMetricSet; | ||
| childThreadSolverScope.startingSolverCount = startingSolverCount; | ||
| // Experiments show that this trick to attain reproducibility doesn't break uniform distribution | ||
| var delegatingRandom = (DelegatingSplittableRandomGenerator) workingRandom; | ||
| childThreadSolverScope.workingRandom = | ||
| var delegatingRandom = (DelegatingSplittableRandomGenerator) moveGeneratorWorkingRandom; | ||
| childThreadSolverScope.moveGeneratorWorkingRandom = | ||
| new DelegatingSplittableRandomGenerator(delegatingRandom.getSeed(), delegatingRandom.split()); | ||
| childThreadSolverScope.solverWorkingRandom = delegatingRandom.split(); | ||
| childThreadSolverScope.scoreDirector = scoreDirector.createChildThreadScoreDirector(childThreadType); | ||
| childThreadSolverScope.startingSystemTimeMillis.set(startingSystemTimeMillis.get()); | ||
| resetAtomicLongTimeMillis(childThreadSolverScope.endingSystemTimeMillis); | ||
|
|
||
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 would move scope need to expose this?
Generally I'd think twice about which of these randoms we make accessible in which circumstances - what a user (or a developer in this case) can't see cannot confuse them and can't be misused.