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.

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

Dev restart #950

Merged
merged 6 commits into from
Oct 18, 2022
Merged

Dev restart #950

merged 6 commits into from
Oct 18, 2022

Conversation

cprudhom
Copy link
Member

This branch is dedicated to a small revamp of the way restarts are managed.
The main idea is to avoid using a Move implementation to deal with restarts but to integrate this directly in the Solver object. Doing so, it becomes easier (well, doable) to remove or replace a restart policy.
In addition, strategies and variable selectors are instrumented to indicate if they are dynamic, that is learn-based objects that are to be combined with restarts to be more efficient. A warning is printed to the console (if warnings are enabled) to indicate poor combinations (such as dyn. strat. without restarts).

…d in Solver

In consequence, it is easy to set/clear the restart policy.
@cprudhom cprudhom added this to the 4.10.11 milestone Oct 14, 2022
@sonatype-lift
Copy link

sonatype-lift bot commented Oct 14, 2022

⚠️ 87 God Classes were detected by Lift in this project. Visit the Lift web console for more details.


public void setNext(AbstractRestart restart) {
throw new UnsupportedOperationException("Cannot set next on this instance");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not putting the nextrestart variable in the abstract class?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right.
Well, that was because NO_RESTART singleton must not declare next, but I can manage that differently.

* @author Charles Prud'homme
* @since 03/09/2015
*/
public final class Restarter extends AbstractRestart {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit from declaring this class final?

* Indicates if this strategy is learn-based may, for instance, require to be coupled with a restart strategy.
* @return <i>true</i> if this strategy is dynamic.
*/
public abstract boolean isDynamic();
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the concept of dynamic search quite vague and possibly confusing.
If it is really about restarts i would suggest a more explicit name, like isImprovedByRestarts.
However, i am not 100% convinced by the idea of this feature, because it often depends on the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, the choice of the term and the concept it encapsulates are debatable.
However, it happens quite often (and this is notably the case of the default strategy 🤦) that the strategy/restarts couple is unsuitable.

However, this can be the subject of another RP with a more in-depth discussion.

@@ -104,6 +104,11 @@ public boolean init() {
return mainStrategy.init();
}

@Override
public boolean isDynamic() {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

For instance here it is not so clear to me that we should recommend restarts

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I asked myself the same question and made the choice that seemed most logical.

@jgFages
Copy link
Contributor

jgFages commented Oct 15, 2022

Hi, I find the refactoring of the restart a very good idea, because it is indeed sometimes hard to follow.

What would the code look like to restart both on solutions and every 100 fails for instance?

I am just not so sure about the isDynamical function.
I feel it is not so easy to answer true or false to that question (exept for inputOrderLB). So i wonder if it is worth increasing the codebase, especially if it is just for a warning.

@cprudhom
Copy link
Member Author

What would the code look like to restart both on solutions and every 100 fails for instance?

solver.setLubyRestart(100, new FailCounter(target, 0), 5000);
solver.setRestartOnSolutions();

I agree that the 2nd point must be discussed (somewhere else)

@jgFages
Copy link
Contributor

jgFages commented Oct 17, 2022

Thanks for your answers!

What would the code look like to restart both on solutions and every 100 fails for instance?

solver.setLubyRestart(100, new FailCounter(target, 0), 5000);
solver.setRestartOnSolutions();

Before we had java ref().setMove(new MoveRestart(ref().getMove() , so the "set" was actually an add operation -> we had in the end the two restart strategies.

However, the new implementation looks like a real setter (the new Restarter will replace the previous one).
It is more consistent with the method name, but I feel the luby restart will be erased by the setRestartOnSolutions.
I would expect the two restarts to be linked by a call to setNext()... Maybe I missed something

@cprudhom
Copy link
Member Author

True, that was my first intention but I didn't go through with it
I'll fix that.
I can do that with a factory, which may ease the definition of the restarter (and we can keep the setRestarter method as is) and hides implementation details.

@cprudhom
Copy link
Member Author

cprudhom commented Oct 17, 2022

Proposal :

Prop 0 (current):

model.getSolver().setLubyRestart(2, new FailCounter(model, 2), 25000);
model.getSolver().setRestartOnSolutions();

Prop 1:

model.getSolver().makeRestarter().luby(2).onFailure().upTo(25000).restartOnSolution();

Prop 2:

model.getSolver().setLubyRestart(2, new FailCounter(model, 2), 25000, true);

where true indicates that restart on solution is required.

I can keep Prop 0, but I think there is a side effect which allows to combine n restarts policy inadvertently (actually, I can add checks).
Prop 1 is quite verbose.
Prop 2 is not very flexible.

jgFages
jgFages previously approved these changes Oct 17, 2022
@mergify mergify bot dismissed jgFages’s stale review October 17, 2022 14:19

Pull request has been modified.

@cprudhom cprudhom self-assigned this Oct 18, 2022
@cprudhom cprudhom merged commit 83122fa into master Oct 18, 2022
@cprudhom cprudhom deleted the dev_restart branch October 18, 2022 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants