feat: improve unassign move generation#2219
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a local-search edge case where the list change destination selector could appear exhausted when a randomly selected value has no reachable (unpinned) entities, by adjusting iterator hasNext()/fallback behavior to keep the selector active and allow unassign destinations.
Changes:
- Update
ElementPositionRandomIteratorto consider the presence of assigned values when determininghasNext(), and to fall back to unassigned destinations when the entity iterator is exhausted. - Reset more internal state in
UpcomingSelectionIterator.discardUpcomingSelection()to support proper discarding behavior. - Add/extend tests and test utilities to cover pinned/unassigned and discard/reset scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java | Updates random destination iteration logic to avoid premature exhaustion and to support unassign fallback. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java | Changes discard behavior to reset internal “upcoming” state for reuse. |
| core/src/test/java/ai/timefold/solver/core/testdomain/list/TestdataListUtils.java | Adds a mocked UpcomingSelectionIterator helper for deterministic iterator-state testing. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java | Adds regression tests for fully pinned entities and iterator discard/reset behavior. |
.../timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java
Outdated
Show resolved
Hide resolved
.../timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java
Outdated
Show resolved
Hide resolved
...java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java
Outdated
Show resolved
Hide resolved
.../timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java
Show resolved
Hide resolved
...in/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelector.java
Outdated
Show resolved
Hide resolved
...java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java
Show resolved
Hide resolved
...java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java:55
- UpcomingSelectionIterator.next() may call createUpcomingSelection() twice after discardUpcomingSelection() (recheckUpcomingSelection=true and upcomingCreated=false), potentially skipping a selection; set upcomingCreated=true in the recheck block or otherwise ensure createUpcomingSelection() is invoked only once per returned element.
public S next() {
if (recheckUpcomingSelection) {
upcomingSelection = createUpcomingSelection();
// We ensure that the variable remains consistent if "discardUpcomingSelection" was called previously.
hasUpcomingSelection = upcomingSelection != null;
recheckUpcomingSelection = false;
}
if (!hasUpcomingSelection) {
throw new NoSuchElementException();
}
if (!upcomingCreated) {
upcomingSelection = createUpcomingSelection();
}
...java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java
Show resolved
Hide resolved
...in/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelector.java
Outdated
Show resolved
Hide resolved
...ava/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java
Outdated
Show resolved
Hide resolved
|



The list change move selector may be prematurely removed from the union move selector iterator during a given step when it picks a random value and all reachable entities from that value are pinned, leaving no valid entities available.
We are not verifying whether any values are assigned, so at least one valid destination could be generated. This PR updates the
hasNextlogic of the iterator to address that. One downside of configuring the list change selector to return true whenever there are any assigned values is that it can generate no-change moves, which waste CPU cycles. This situation can occur if the planning value is unassigned and all of its reachable entities are pinned.Some experiments found no regressions from this change in the FSR.