feat(neighborhoods): introduce coarse-grained generic moves - #2610
Open
triceo wants to merge 16 commits into
Open
feat(neighborhoods): introduce coarse-grained generic moves#2610triceo wants to merge 16 commits into
triceo wants to merge 16 commits into
Conversation
triceo
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces coarse-grained generic neighborhood moves for basic and list variables, with supporting preview APIs, migration recipes, tests, and documentation.
Changes:
- Adds pillar, subpillar, mass, list, and sublist moves and providers.
- Adds sampling, dataset, range, and neighborhood-stream infrastructure.
- Adds preview migration support and upgrade documentation.
Four moderate findings remain unresolved.
Reviewed changes
Copilot reviewed 155 out of 155 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Reviewed change |
|---|---|
tools/migration/src/test/java/ai/timefold/solver/migration/preview/NeighborhoodsMigrationRecipeTest.java |
Migration recipe tests |
tools/migration/src/main/java/module-info.java |
Migration module exports |
tools/migration/src/main/java/ai/timefold/solver/migration/v2/ToLatestV2Recipe.java |
V2 migration composition |
tools/migration/src/main/java/ai/timefold/solver/migration/v1/ToLatestV1Recipe.java |
V1 migration composition |
tools/migration/src/main/java/ai/timefold/solver/migration/ToLatestRecipe.java |
Aggregate migration composition |
tools/migration/src/main/java/ai/timefold/solver/migration/preview/PreviewToLatestRecipe.java |
Preview migration aggregation |
tools/migration/src/main/java/ai/timefold/solver/migration/preview/package-info.java |
Preview migration documentation |
tools/migration/src/main/java/ai/timefold/solver/migration/preview/NeighborhoodsMigrationRecipe.java |
Neighborhood API migration |
docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-to-latest.adoc |
Latest upgrade guide |
docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-from-v1.adoc |
V1 upgrade documentation |
docs/src/modules/ROOT/pages/upgrading-timefold-solver/backwards-compatibility.adoc |
Compatibility documentation |
docs/src/modules/ROOT/pages/upgrading-timefold-solver/.upgrading-timefold-solver.adoc |
Upgrade page inclusion |
docs/src/modules/ROOT/nav.adoc |
Documentation navigation |
core/src/test/java/ai/timefold/solver/core/testutil/NeighborhoodTestUtils.java |
Neighborhood test utilities |
core/src/test/java/ai/timefold/solver/core/testdomain/valuerange/entityproviding/multivar/TestdataAllowsUnassignedMultiVarEntityProvidingSolution.java |
Multivariable test data |
core/src/test/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/SampleTest.java |
Sample API tests |
core/src/test/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/DatasetTest.java |
Dataset API tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/SwapMoveTest.java |
Swap move tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/SubPillarUnassignMoveProviderTest.java |
Subpillar unassignment tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/SampleValueRangesBiasIT.java |
Value-range bias tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/MassUnassignMoveProviderTest.java |
Mass unassignment tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/MassAssignMoveProviderTest.java |
Mass assignment tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/ListSwapMoveProviderTest.java |
List swap provider tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/ListChangeMoveProviderTest.java |
List change provider tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/ChangeMoveTest.java |
Change move tests |
core/src/test/java/ai/timefold/solver/core/preview/api/move/builtin/ChangeMoveProviderTest.java |
Change provider tests |
core/src/test/java/ai/timefold/solver/core/impl/util/TriangularNumbersTest.java |
Triangular-number tests |
core/src/test/java/ai/timefold/solver/core/impl/util/TriangleElementFactoryTest.java |
Triangle factory tests |
core/src/test/java/ai/timefold/solver/core/impl/neighborhood/bias/SamplingIteratorBiasIT.java |
Sampling bias tests |
core/src/test/java/ai/timefold/solver/core/impl/neighborhood/bias/PairFairnessBiasIT.java |
Pair fairness tests |
core/src/test/java/ai/timefold/solver/core/impl/neighborhood/bias/DatasetBucketBiasIT.java |
Dataset bucket bias tests |
core/src/test/java/ai/timefold/solver/core/impl/neighborhood/bias/BiasReport.java |
Bias reporting |
core/src/test/java/ai/timefold/solver/core/impl/neighborhood/bias/AbstractBiasIT.java |
Shared bias test support |
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/RandomSubListSwapMoveSelectorTest.java |
Random sublist swap selector tests |
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/RandomSubListChangeMoveSelectorTest.java |
Random sublist change selector tests |
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/RandomSubListSelectorTest.java |
Random sublist selector tests |
core/src/main/java/module-info.java |
Core module exports |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/UniMoveConstructor.java |
Unary move construction |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/picking/UniPickingStream.java |
Unary picking stream |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/picking/PickingStream.java |
Picking stream API |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/picking/BiPickingStream.java |
Binary picking stream |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/MoveStreamFactory.java |
Move stream factory |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/enumerating/UniEnumeratingStream.java |
Unary enumeration stream |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/enumerating/BiEnumeratingStream.java |
Binary enumeration stream |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/UniDatasetInstance.java |
Unary dataset instances |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/SubListSampler.java |
Sublist sampling |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/Sampler.java |
Sampler API |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/Sample.java |
Sample API |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/Range.java |
Range API |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/package-info.java |
Sample API documentation |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/DefaultSample.java |
Sample implementation |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/DefaultRange.java |
Range implementation |
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/BiDatasetInstance.java |
Binary dataset instances |
core/src/main/java/ai/timefold/solver/core/preview/api/move/MutableSolutionView.java |
Mutable solution access |
core/src/main/java/ai/timefold/solver/core/preview/api/move/Move.java |
Move contract |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/UnassignMoveProvider.java |
Unassignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/SwapMoveProvider.java |
Swap move provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/SwapMove.java |
Swap move implementation |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/SubListUnassignMove.java |
Sublist unassignment |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/SubListChangeMove.java |
Sublist change move |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/PillarUnassignMoveProvider.java |
Pillar unassignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/package-info.java |
Built-in move documentation |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassUnassignMoveProvider.java |
Mass unassignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListUnassignMoveProvider.java |
Mass list unassignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListDestinationMoveIterator.java |
Mass list destination iteration |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListChangeMoveProvider.java |
Mass list change provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListChangeMove.java |
Mass list change move |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListAssignMoveProvider.java |
Mass list assignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassChangeMoveProvider.java |
Mass change provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassChangeMove.java |
Mass change move |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassAssignMoveProvider.java |
Mass assignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListUnassignMoveProvider.java |
List unassignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListUnassignMove.java |
List unassignment move |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListSwapMoveProvider.java |
List swap provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListAssignMoveProvider.java |
List assignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListAssignMove.java |
List assignment move |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ChangeMoveProvider.java |
Change move provider |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ChangeMove.java |
Change move implementation |
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/AssignMoveProvider.java |
Assignment provider |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/VariableMetaModel.java |
Variable metadata |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/ShadowVariableMetaModel.java |
Shadow variable metadata |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/ShadowEntityMetaModel.java |
Shadow entity metadata |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/PlanningEntityMetaModel.java |
Planning entity metadata |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/GenuineVariableMetaModel.java |
Genuine variable metadata |
core/src/main/java/ai/timefold/solver/core/preview/api/domain/metamodel/ElementPosition.java |
Element position metadata |
core/src/main/java/ai/timefold/solver/core/impl/util/TriangularNumbers.java |
Triangular-number utilities |
core/src/main/java/ai/timefold/solver/core/impl/util/TriangleElementFactory.java |
Triangle element factory |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/sampling/InnerUniSamplingStream.java |
Unary sampling implementation |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/sampling/InnerSamplingStream.java |
Sampling implementation |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/picking/InnerUniPickingStream.java |
Unary picking implementation |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/picking/InnerPickingStream.java |
Picking implementation |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/picking/DefaultUniPickingStream.java |
Default unary picking |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/picking/DefaultBiPickingStream.java |
Default binary picking |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/enumerating/common/AbstractLeftDatasetInstance.java |
Dataset join support |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/DefaultMoveStreamFactory.java |
Move stream construction |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/dataset/sample/SampleAssembler.java |
Sample assembly |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/dataset/JustInTimeBiDatasetInstance.java |
Just-in-time binary datasets |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/stream/dataset/DefaultUniDatasetInstance.java |
Default unary datasets |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/DefaultNeighborhoodTestContext.java |
Neighborhood test context |
core/src/main/java/ai/timefold/solver/core/impl/neighborhood/DefaultNeighborhoodProvider.java |
Neighborhood provider |
core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/TriangleElementFactory.java |
List selection factory |
core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/RandomSubListSelector.java |
Random sublist selection |
core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/NullAllowingValueRange.java |
Null-allowing value ranges |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java |
Solution metadata |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/InnerPlanningEntityMetaModel.java |
Planning entity metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DefaultShadowVariableMetaModel.java |
Shadow variable metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DefaultShadowEntityMetaModel.java |
Shadow entity metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DefaultPlanningVariableMetaModel.java |
Planning variable metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DefaultPlanningListVariableMetaModel.java |
Planning list metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DefaultGenuineEntityMetaModel.java |
Genuine entity metadata construction |
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/index/RetiringRandomIterator.java |
Random iterator support |
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/index/MappingRetiringRandomIterator.java |
Mapped random iterator support |
CONSTITUTION.md |
Repository guidance |
Suppressed comments (7)
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MassListChangeMove.java:79
Sampleexplicitly allows null members, but this move dereferences every member here (and again ingetPlanningValues()), so a validSamplecan make the public list mass-change move fail with an unhelpfulNullPointerException; either reject null members when constructing the move or document a non-null-member precondition.
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/MoveProviderUtil.java:232- The provider groups all entities, including those whose variable is null, but
containsInEveryis called on ranges with nullable wrappers removed, so legal swaps involving an unassigned value are always rejected; handle null as legal whenvariableMetaModel.allowsUnassigned()is true.
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/PillarChangeMoveProvider.java:42 - The new Javadoc contains the truncated word
winstead ofwould, making the explanation grammatically incorrect.
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/SampleValueRanges.java:126 ValueRange.getSize()is a long, butadmittedCountis an int, sopickExactlyoverflows after 2^31−1 admitted values and eventually passes a non-positive bound tonextInt; use a long counter andnextLong.
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/DefaultSample.java:48Sampledocuments value-based equality for any two samples, but this implementation only comparesDefaultSample, so a valid customSamplewith the same members is considered unequal; compare against theSampleinterface and preserve the same member-based hash contract.
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/Sample.java:45first()is annotated nullable andDefaultSamplereturns null when null is the first member, but this Javadoc promises a never-null result, which gives callers an unsafe contract; document that the result may be null.
docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-to-latest.adoc:9- This unconditional promise contradicts the preview API migration documented later on this page and the preview compatibility policy: users on preview APIs can be broken by exactly the sampling-to-picking rename; qualify the guarantee to stable APIs or explicitly exclude preview APIs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 199 out of 199 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
core/src/main/java/ai/timefold/solver/core/preview/api/move/builtin/ListSwapMoveProvider.java:58
- ListSwapMoveProvider reuses a PositionInList obtained before unassigning as the subsequent assign target even though ElementPosition is documented as only valid when obtained; pass entity and index to Moves.assign(...) instead of reusing the PositionInList object.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 199 out of 199 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tools/migration/src/main/java/ai/timefold/solver/migration/ToLatestRecipe.java:36
- ToLatestRecipe runs RemoveUnusedImports even though ToLatestV1Recipe and ToLatestV2Recipe already do, which redundantly applies the same cleanup multiple times during a single migration run.
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/dataset/sample/SubListSampler.java:24 - The caveat says fully
@PlanningPinentities report getFirstUnpinnedIndex as 0, which contradicts SolutionView#getFirstUnpinnedIndex Javadoc claiming the value is at most countValues when the entire list is pinned.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 203 out of 203 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/enumerating/BiEnumeratingStream.java:35
- The new concat(UniEnumeratingStream, Function) method is missing @param/@return Javadoc tags required by CONSTITUTION.md for public APIs, so please add them.
core/src/main/java/ai/timefold/solver/core/preview/api/neighborhood/stream/enumerating/UniEnumeratingStream.java:48 - The new concat(BiEnumeratingStream, Function) method is missing @param/@return Javadoc tags required by CONSTITUTION.md for public APIs, so please add them.
Comment on lines
+26
to
+29
| /** | ||
| * As defined by {@link UniEnumeratingStream#concat(UniEnumeratingStream)}. | ||
| */ | ||
| BiEnumeratingStream<Solution_, A, B> concat(BiEnumeratingStream<Solution_, A, B> otherStream); |
Comment on lines
+37
to
+40
| * | ||
| * @return a stream containing every tuple of both streams | ||
| */ | ||
| UniEnumeratingStream<Solution_, A> concat(UniEnumeratingStream<Solution_, A> otherStream); |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 204 out of 204 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
core/src/main/java/ai/timefold/solver/core/api/domain/common/Lookup.java:40
- lookUpNonNullWorkingObject(T) can still return null because it doesn’t null-check the lookup result, which breaks its contract and can cause unexpected NPEs in Move.rebase callers; wrap the lookUpWorkingObject(...) result in Objects.requireNonNull(...).
/**
* As defined by {@link #lookUpWorkingObject(Object)},
* but does not accept null arguments and cannot return null.
*/
default <T> T lookUpNonNullWorkingObject(T problemFactOrPlanningEntity) {
return lookUpWorkingObject(Objects.requireNonNull(problemFactOrPlanningEntity));
}
Comment on lines
+38
to
+40
| default <T> T lookUpNonNullWorkingObject(T problemFactOrPlanningEntity) { | ||
| return lookUpWorkingObject(Objects.requireNonNull(problemFactOrPlanningEntity)); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces:
Documentation will only be written after we agree on all the things that are in here. The chances of having to redo the docs because of review changes are too big.
My recommendation for review: start with the moves and move providers. Understand them. Before we discuss the implementation, we need to be in agreement that we want these moves, and not some others. (The implementation is dictated by the requirements of the moves - therefore if we decide to change the moves, we will change the implementation.)