fix: seperate move and solver RandomGenerator usage#2356
fix: seperate move and solver RandomGenerator usage#2356Christopher-Chianelli wants to merge 1 commit into
Conversation
Before, some foragers and acceptors used the same RandomGenerator as moves, which can affect reproducibility when multithreading is used. Now, foragers and acceptors use their own independent random.
Christopher-Chianelli
temporarily deployed
to
internal
GitHub Actions
Inactive
|
There was a problem hiding this comment.
I think we need more clarity around this concept; as it stands, it is confusing and I smell future problems here.
Is it a naming issue? Is it a documentation issue? Is it a scope/visibility issue? Hard to say. I'd start by figuring out the right naming, and very clearly defining the demarcation line - what do I use one random for, and what the other?
One problem I have is that - once you have the random - you don't really know anymore what random you have. There's no telling. Maybe instead of having two random instances, we update the internal type to carry the two randoms internally and user would have to specifically request the one which they need?
Something like var number = ourCustomRandom.forMoves(r -> r.nextInt());. It's not impossible to store r and reuse it later, but it makes it sufficiently difficult plus it's always very clear that r came from forMoves because the call site will likely be close in code to the store site.
Exploring that idea further, the lambda there could be a problem, as very often it would be capturing and we don't like that. So, assuming that we only have one instance of ourCustomRandom, the best we can do is to have ourCustomRandom.forMoves().nextInt(bound); users can easily store the child random and reuse it, but we control the code and we can kill that idea in code review.
This to me seems like a better approach than to have two RNG instances floating around the solver without having any way to identify which is which. This way, we will be able to immediately verify and look up in the IDE which pieces of code are using which RNG, simply because each call site would be using forMoves() or forSolver() (TODO naming).
| /** | ||
| * Only used for tests | ||
| */ | ||
| public void setTestWorkingRandom(RandomGenerator workingRandom) { | ||
| this.moveGeneratorWorkingRandom = workingRandom; | ||
| this.solverWorkingRandom = workingRandom; | ||
| } |
There was a problem hiding this comment.
Please don't. Find another way. Public methods on types as prominent as this must not be only used for testing.
| public RandomGenerator getMoveGeneratorWorkingRandom() { | ||
| return moveGeneratorWorkingRandom; | ||
| } | ||
|
|
||
| public void setWorkingRandom(RandomGenerator workingRandom) { | ||
| this.workingRandom = workingRandom; | ||
| public RandomGenerator getSolverWorkingRandom() { | ||
| return solverWorkingRandom; | ||
| } |
There was a problem hiding this comment.
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.
| public RandomGenerator getSolverWorkingRandom() { | ||
| return getStepScope().getSolverWorkingRandom(); | ||
| } |
There was a problem hiding this comment.
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.
| protected final List<Phase<Solution_>> phaseList; | ||
|
|
||
| private RandomGenerator.@Nullable SplittableGenerator savedRandom; | ||
| private RandomGenerator.@Nullable SplittableGenerator savedMoveGeneratorRandom; |
There was a problem hiding this comment.
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.
2 participants
Before, some foragers and acceptors used the same RandomGenerator as moves, which can affect reproducibility when multithreading is used.
Now, foragers and acceptors use their own independent random.