-
Notifications
You must be signed in to change notification settings - Fork 149
chore: create filtering value range selectors as early as possible #1738
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?
Conversation
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.
Good start! Any performance numbers?
core/src/main/java/ai/timefold/solver/core/impl/domain/entity/descriptor/EntityDescriptor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/domain/policy/DescriptorPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/domain/policy/DescriptorPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/domain/policy/DescriptorPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java
Outdated
Show resolved
Hide resolved
...old/solver/core/impl/heuristic/selector/move/generic/list/ListChangeMoveSelectorFactory.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java
Outdated
Show resolved
Hide resolved
...mefold/solver/core/impl/heuristic/selector/move/generic/list/ListChangeMoveSelectorTest.java
Show resolved
Hide resolved
e74a85e
to
0130996
Compare
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.
Getting closer.
Are we sure this doesn't change the moves that are generated? I mean - of course it does, that's the point, but are we sure no moves are lost that should not have been lost?
It is difficult to evaluate the logic of the move selector structure.
core/src/main/java/ai/timefold/solver/core/impl/domain/policy/DescriptorPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/domain/policy/DescriptorPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java
Outdated
Show resolved
Hide resolved
.../main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/EntitySelectorFactory.java
Show resolved
Hide resolved
...in/java/ai/timefold/solver/core/impl/heuristic/selector/list/DestinationSelectorFactory.java
Show resolved
Hide resolved
...rc/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/ValueSelectorFactory.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeManager.java
Outdated
Show resolved
Hide resolved
Our FSR validation shows that invalid moves are no longer generated and the overall solution quality has improved, indicating that no valid moves are missed. Also, the coverage using the original iterators confirms that all expected moves are generated, and since the random iterators rely on the same approach, I don't think that's a problem. |
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
|
This PR updates the value and entity factories to create the entity filtering-related selectors as early as possible. This way, the reachable values are returned first and then processed by outer node selectors. Additionally, it also includes a small change to minimize the hash lookup.
(The next PR will focus on these hash lookups)