Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add new reconfiguration process #1352

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

feat: add new reconfiguration process #1352

wants to merge 22 commits into from
+664 −28

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented Jan 24, 2025 •
edited
Loading

This pull request introduces a restart mechanism for the LS phase and a reconfiguration process.

  1. There is one restart mechanism based on the unimproved move count of the best solution.
  2. The restart logic employs a technique known as geometric restart, which increases the related metric exponentially.
  3. The reconfiguration process is executed by the LS every time a restart is triggered. Currently, the process simply restores the last best solution and "restarts" the search.

@zepfred zepfred changed the title feat: add Local Search reconfiguration process feat: add new reconfiguration process Jan 24, 2025
@zepfred zepfred requested a review from triceo January 24, 2025 17:54
@zepfred zepfred marked this pull request as ready for review January 24, 2025 17:54
triceo
triceo reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor

@triceo triceo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

First round of review is in.

My most general comment is: we need a different name for "reconfiguration". I know that the papers use this word, but we cannot. We already have "configuration", and using "reconfiguration" would be confusing - the "configuration" in "reconfiguration" has nothing to do with solver configuration.

Reheating? Restart? Refresh? Reboot? Let's discuss.

core/src/main/java/ai/timefold/solver/core/config/solver/PreviewFeature.java Outdated Show resolved Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/LocalSearchDecider.java Outdated Show resolved Hide resolved
...n/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/ReconfigurableAcceptor.java Outdated Show resolved Hide resolved
...n/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/ReconfigurableAcceptor.java Outdated Show resolved Hide resolved
.../solver/core/impl/localsearch/decider/acceptor/restart/AbstractGeometricRestartStrategy.java Outdated Show resolved Hide resolved
...i/timefold/solver/core/impl/localsearch/decider/reconfiguration/ReconfigurationStrategy.java Outdated Show resolved Hide resolved
...ore/impl/localsearch/decider/reconfiguration/RestoreBestSolutionReconfigurationStrategy.java Outdated Show resolved Hide resolved
...ore/impl/localsearch/decider/reconfiguration/RestoreBestSolutionReconfigurationStrategy.java Outdated Show resolved Hide resolved
...ore/impl/localsearch/decider/reconfiguration/RestoreBestSolutionReconfigurationStrategy.java Outdated Show resolved Hide resolved
...test/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/AcceptorFactoryTest.java Show resolved Hide resolved
@triceo
Copy link
Contributor

triceo commented Jan 27, 2025

Another concern: we only support LA (and DLAS). But shouldn't we support simulated annealing too? In SA, the process is actually called reheating, and is well researched as far as I know.

For the record: I don't much care for SA, because it is hard to configure and therefore not much useful. But if we don't support reheating the SA, it should be a conscious decision, and not just something we forgot to do.

@zepfred zepfred had a problem deploying to documentation (preview) January 28, 2025 22:13 — with GitHub Actions Failure
triceo
triceo reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

Next round of comments.

...er/core/impl/localsearch/decider/acceptor/restart/UnimprovedMoveCountStuckCriterionTest.java Show resolved Hide resolved
...main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/RestartableAcceptor.java Outdated Show resolved Hide resolved
...core/impl/localsearch/decider/acceptor/lateacceptance/DiversifiedLateAcceptanceAcceptor.java Outdated Show resolved Hide resolved
...d/solver/core/impl/localsearch/decider/acceptor/restart/AbstractGeometricStuckCriterion.java Outdated
protected final Clock clock;
protected final Logger logger = LoggerFactory.getLogger(AbstractGeometricRestartStrategy.class);
protected final double scalingFactor;
private static final long GRACE_PERIOD_MILLIS = 30_000; // Same value as DiminishedReturnsTermination
Copy link
Contributor

Choose a reason for hiding this comment

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

Questionable. If the diminished termination is enabled, don't we want the solver to be able to restart before the termination triggers? Then arguably this should be less than whatever the termination is configured to be. But I'm not sure if we even want that.

Should we detect the termination in the solver config? Should we depend on whatever values the user has configured there? I'm not sure either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your viewpoint and struggled to select an appropriate default value. That's why I opted for the same value as the diminished termination.

However, I believe we should not rely on that termination at all. This should stand as an independent value.

...d/solver/core/impl/localsearch/decider/acceptor/restart/AbstractGeometricStuckCriterion.java Outdated Show resolved Hide resolved
...solver/core/impl/localsearch/decider/acceptor/restart/UnimprovedMoveCountStuckCriterion.java Outdated Show resolved Hide resolved
...ava/ai/timefold/solver/core/impl/localsearch/decider/reconfiguration/AdaptedSolverScope.java Outdated Show resolved Hide resolved
...n/java/ai/timefold/solver/core/impl/localsearch/decider/reconfiguration/RestartStrategy.java Outdated Show resolved Hide resolved
...solver/core/impl/localsearch/decider/reconfiguration/RestoreBestSolutionRestartStrategy.java Outdated Show resolved Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractPhaseScope.java Outdated Show resolved Hide resolved
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triceo triceo triceo left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zepfred @triceo