Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public abstract class AbstractSelector<Solution_> implements Selector<Solution_>

@Override
public void solvingStarted(SolverScope<Solution_> solverScope) {
workingRandom = solverScope.getWorkingRandom();
workingRandom = solverScope.getMoveGeneratorWorkingRandom();
phaseLifecycleSupport.fireSolvingStarted(solverScope);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public boolean isNeverEnding() {
public void solvingStarted(SolverScope<Solution_> solverScope) {
super.solvingStarted(solverScope);
this.solverScope = solverScope;
this.workingRandom = solverScope.getWorkingRandom();
this.workingRandom = solverScope.getMoveGeneratorWorkingRandom();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void solvingStarted(SolverScope<Solution_> solverScope) {
this.listVariableStateSupply = solverScope.getScoreDirector()
.getSupplyManager()
.demand(listVariableDescriptor.getStateDemand());
this.workingRandom = solverScope.getWorkingRandom();
this.workingRandom = solverScope.getMoveGeneratorWorkingRandom();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public boolean isAccepted(LocalSearchMoveScope<Solution_> moveScope) {
}
acceptChance *= acceptChanceLevel;
}
return moveScope.getWorkingRandom().nextDouble() < acceptChance;
return moveScope.getSolverWorkingRandom().nextDouble() < acceptChance;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public boolean isAccepted(LocalSearchMoveScope<Solution_> moveScope) {
return false;
}
var acceptChance = calculateFadingTabuAcceptChance(tabuStepCount - workingTabuSize);
var accepted = moveScope.getWorkingRandom().nextDouble() < acceptChance;
var accepted = moveScope.getSolverWorkingRandom().nextDouble() < acceptChance;
if (accepted) {
logger.trace("{} Proposed move ({}) is fading tabu with acceptChance ({}) and is accepted.",
logIndentation,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public LocalSearchMoveScope<Solution_> pickMove(LocalSearchStepScope<Solution_>
if (finalistList.size() == 1 || !breakTieRandomly) {
return finalistList.get(0);
}
int randomIndex = stepScope.getWorkingRandom().nextInt(finalistList.size());
int randomIndex = stepScope.getSolverWorkingRandom().nextInt(finalistList.size());
return finalistList.get(randomIndex);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void solvingStarted(SolverScope<Solution_> solverScope) {

@Override
public void phaseStarted(AbstractPhaseScope<Solution_> phaseScope) {
workingRandom = phaseScope.getWorkingRandom();
workingRandom = phaseScope.getMoveGeneratorWorkingRandom();
phaseScope.getScoreDirector().setMoveRepository(this);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,12 @@ public Solution_ getWorkingSolution() {
return getStepScope().getWorkingSolution();
}

public RandomGenerator getWorkingRandom() {
return getStepScope().getWorkingRandom();
public RandomGenerator getMoveGeneratorWorkingRandom() {
return getStepScope().getMoveGeneratorWorkingRandom();
}

public RandomGenerator getSolverWorkingRandom() {
return getStepScope().getSolverWorkingRandom();
}
Comment on lines +73 to 75

Copy link
Copy Markdown
Collaborator

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.


@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,12 @@ public <Score_ extends Score<Score_>> void assertShadowVariablesAreNotStale(Inne
innerScoreDirector.assertShadowVariablesAreNotStale(workingScore, completedAction);
}

public RandomGenerator getWorkingRandom() {
return getSolverScope().getWorkingRandom();
public RandomGenerator getMoveGeneratorWorkingRandom() {
return getSolverScope().getMoveGeneratorWorkingRandom();
}

public RandomGenerator getSolverWorkingRandom() {
return getSolverScope().getSolverWorkingRandom();
}

public boolean isBestSolutionInitialized() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,12 @@ public Solution_ getWorkingSolution() {
return getPhaseScope().getWorkingSolution();
}

public RandomGenerator getWorkingRandom() {
return getPhaseScope().getWorkingRandom();
public RandomGenerator getMoveGeneratorWorkingRandom() {
return getPhaseScope().getMoveGeneratorWorkingRandom();
}

public RandomGenerator getSolverWorkingRandom() {
return getPhaseScope().getSolverWorkingRandom();
}

public Solution_ cloneWorkingSolution() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import ai.timefold.solver.core.impl.phase.Phase;
import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
import ai.timefold.solver.core.impl.score.director.ScoreDirectorFactory;
import ai.timefold.solver.core.impl.solver.random.DelegatingSplittableRandomGenerator;
import ai.timefold.solver.core.impl.solver.recaller.BestSolutionRecaller;
import ai.timefold.solver.core.impl.solver.scope.SolverScope;
import ai.timefold.solver.core.impl.solver.termination.BasicPlumbingTermination;
Expand Down Expand Up @@ -188,7 +189,7 @@ public void outerSolvingStarted(SolverScope<Solution_> solverScope) {
solving.set(true);
basicPlumbingTermination.resetTerminateEarly();
solverScope.setStartingSolverCount(0);
solverScope.setWorkingRandom(randomFactory.get());
solverScope.setWorkingRandom((DelegatingSplittableRandomGenerator) randomFactory.get());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two need to get very good Javadocs.
Who's supposed to use them? What for? What not for? Why?

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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")
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public static <Solution_, Score_ extends Score<Score_>> SolverScope<Solution_> s
Selector<Solution_>... selectors) {
SolverScope<Solution_> solverScope = new SolverScope<>();
solverScope.setScoreDirector(scoreDirector);
solverScope.setWorkingRandom(random);
solverScope.setTestWorkingRandom(random);
listener.solvingStarted(solverScope);
if (selectors != null) {
for (var selector : selectors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ public void runRandomWithoutEntityListDirtyAndMinimumCacheType(SelectionCacheTyp
Random workingRandom = new TestRandom(1, 0, 0, 2, 1, 2, 2, 1, 0);

SolverScope solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getScoreDirector()).thenReturn(scoreDirector);
entitySelector.solvingStarted(solverScope);

Expand Down Expand Up @@ -283,7 +283,7 @@ void randomWithEntityListDirty() {
Random workingRandom = new TestRandom(1, 0, 0, 2, 1, 2, 2, 1, 0);

SolverScope solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getScoreDirector()).thenReturn(scoreDirector);
entitySelector.solvingStarted(solverScope);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void randomSelection() {
1199.0 / 1234.0);

SolverScope solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
entitySelector.solvingStarted(solverScope);
AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
entitySelector.phaseStarted(phaseScopeA);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ void randomWithSubs() {
TestRandom workingRandom = new TestRandom(0);

SolverScope<TestdataSolution> solverScope = mockSolverScope();
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
pillarSelector.solvingStarted(solverScope);

AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
Expand Down Expand Up @@ -265,7 +265,7 @@ void randomWithSubs_Size2() {
0, 0, 0, 0); // [b, d]

SolverScope<TestdataSolution> solverScope = mockSolverScope();
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
pillarSelector.solvingStarted(solverScope);

AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
Expand Down Expand Up @@ -315,7 +315,7 @@ void sequentialUnlimited() {
);

SolverScope<TestdataSolution> solverScope = mockSolverScope();
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
pillarSelector.solvingStarted(solverScope);

AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ void randomPartiallyPinnedAndUnassigned() {

var solverScope = new SolverScope<TestdataPinnedUnassignedValuesListSolution>();
solverScope.setScoreDirector(scoreDirector);
solverScope.setWorkingRandom(random);
solverScope.setTestWorkingRandom(random);
var entitySelector = new FromSolutionEntitySelector<>(
solutionDescriptor.findEntityDescriptorOrFail(TestdataPinnedUnassignedValuesListEntity.class),
SelectionCacheType.PHASE, true);
Expand Down Expand Up @@ -415,7 +415,7 @@ void randomUnassignedSingleEntity() {
solverScope.setScoreDirector(scoreDirector);
// This needs to use a real Random instance, otherwise the test never covers the situation where
// the random value of 1 needs to be produced to get to the entity.
solverScope.setWorkingRandom(new Random(0));
solverScope.setTestWorkingRandom(new Random(0));
var entitySelector = new FromSolutionEntitySelector<>(
solutionDescriptor.findEntityDescriptorOrFail(TestdataAllowsUnassignedValuesListEntity.class),
SelectionCacheType.PHASE, true);
Expand Down Expand Up @@ -467,7 +467,7 @@ void randomFullyPinned() {

var solverScope = new SolverScope<TestdataPinnedWithIndexListSolution>();
solverScope.setScoreDirector(scoreDirector);
solverScope.setWorkingRandom(random);
solverScope.setTestWorkingRandom(random);
selector.solvingStarted(solverScope);
selector.phaseStarted(new LocalSearchPhaseScope<>(solverScope, 0));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ void biasedRandomSelection() {
0.0,
999.0 / 1020.0);
SolverScope<TestdataSolution> solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);
AbstractPhaseScope<TestdataSolution> phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
moveSelector.phaseStarted(phaseScopeA);
Expand Down Expand Up @@ -133,7 +133,7 @@ void uniformRandomSelection() {

Random workingRandom = new TestRandom(0, 1, 1, 0, 0);
SolverScope<TestdataSolution> solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);
AbstractPhaseScope<TestdataSolution> phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
moveSelector.phaseStarted(phaseScopeA);
Expand Down Expand Up @@ -162,7 +162,7 @@ void emptyUniformRandomSelection() {
Random workingRandom = new TestRandom(1);

SolverScope<TestdataSolution> solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);
AbstractPhaseScope<TestdataSolution> phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
moveSelector.phaseStarted(phaseScopeA);
Expand Down Expand Up @@ -195,7 +195,7 @@ void emptyBiasedRandomSelection() {
Random workingRandom = new TestRandom(1);

SolverScope<TestdataSolution> solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);
AbstractPhaseScope<TestdataSolution> phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
moveSelector.phaseStarted(phaseScopeA);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public void runRandomSelection(SelectionCacheType cacheType, int timesCalled) {
TestRandom workingRandom = new TestRandom(1, 0, 2);

SolverScope solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);

AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void randomSelection() {
1199.0 / 1234.0);

SolverScope<TestdataSolution> solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);
AbstractPhaseScope<TestdataSolution> phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
moveSelector.phaseStarted(phaseScopeA);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public void run(SelectionCacheType cacheType, int timesCalled) {

TestRandom workingRandom = new TestRandom(2, 0);
SolverScope solverScope = mock(SolverScope.class);
when(solverScope.getWorkingRandom()).thenReturn(workingRandom);
when(solverScope.getMoveGeneratorWorkingRandom()).thenReturn(workingRandom);
moveSelector.solvingStarted(solverScope);

AbstractPhaseScope phaseScopeA = PlannerTestUtils.delegatingPhaseScope(solverScope);
Expand Down
Loading
Loading