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

Stopping criteria #64

Merged
merged 14 commits into from
May 17, 2022
Merged

Stopping criteria #64

merged 14 commits into from
May 17, 2022

Conversation

leonlan
Copy link
Collaborator

@leonlan leonlan commented May 17, 2022

Related issue: #62

I had more free time than I predicted :-).

The changes I made mainly follow the same implementation style of the acceptance criteria.

Changes

  • Implement a StoppingCriterion class and provide two default criteria MaxIterations and MaxRuntime
  • Changed the ALNS.iterate interface to take stop: StoppingCriterion parameter as input instead of iterations: int

Motivation behind the StoppingCriterion interface:

  • In particular: StoppingCriterion.__call__(self, best: State, current: State)

  • I searched through the Handbook of Metaheuristics (2019) for termination criteria possibilities. Section 2.3.7. on Tabu Search - Termination criteria gives the following examples:

    One may have noticed that we have not specified in our template above a termination criterion. In theory, the search could go on forever, unless the optimal value of the problem at hand is known beforehand. In practice, obviously, the search has to be stopped at some point. The most commonly used stopping criteria in TS are:
    • after a fixed number of iterations (or a fixed amount of CPU time);
    • after some number of iterations without an improvement in the objective function
    value (the criterion used in most implementations);
    • when the objective reaches a pre-specified threshold value.

  • I based my implementation to take into account these points. The first point is addressed by the implementation of MaxIterations and MaxRuntime. The second and third point can be implemented based on this interface, which would not have been possible if __call__ does not accept the best and current solution.

Remarks

  • I didn't know what to do with the linting/formatting since it wasn't specified in pyproject.toml.
  • I only changed the tests to make sure they all passed due to the new iterate interface, but I haven't changed the documentation/examples yet. I can do this after we've decided on the proposed changes.

@leonlan
Copy link
Collaborator Author

leonlan commented May 17, 2022

Seems that the static analysis is failing for MaxRuntime because of how I'm using the properties. I don't know what the correct practice is here. Any suggestions?

alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/StoppingCriterion.py Outdated Show resolved Hide resolved
alns/stopping_criteria/tests/test_max_runtime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxIterations.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
alns/stopping_criteria/MaxRuntime.py Outdated Show resolved Hide resolved
@N-Wouda
Copy link
Owner

N-Wouda commented May 17, 2022

@leonlan thanks for your PR. I just left some comments, but overall this already looks pretty good to me.

I didn't know what to do with the linting/formatting since it wasn't specified in pyproject.toml.

Thanks for letting me know about this! I have a style that I use, but that style can best be described as "tweaked over the years and baked into my editor". I do not have a formal document describing it, so I will not hold you to what I think looks good. I will look into standardising the style for ALNS sometime soon!

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #64 (0c0de8e) into master (f478e0c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #64   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        19    +4     
  Lines          322       367   +45     
=========================================
+ Hits           322       367   +45     
Impacted Files Coverage Δ
alns/ALNS.py 100.00% <100.00%> (ø)
alns/Statistics.py 100.00% <100.00%> (ø)
alns/stopping_criteria/MaxIterations.py 100.00% <100.00%> (ø)
alns/stopping_criteria/MaxRuntime.py 100.00% <100.00%> (ø)
alns/stopping_criteria/StoppingCriterion.py 100.00% <100.00%> (ø)
alns/stopping_criteria/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f478e0c...0c0de8e. Read the comment docs.

@N-Wouda N-Wouda merged commit 2550a14 into N-Wouda:master May 17, 2022
@N-Wouda
Copy link
Owner

N-Wouda commented May 17, 2022

@leonlan thanks for the PR and your hard work!

I will release this in a few days as v3.0.0. I have to bump the major since this is a breaking change.

@leonlan
Copy link
Collaborator Author

leonlan commented May 17, 2022

Thanks to you for the helpful feedback as well!

I make extensive use of ALNS and I’m really grateful for the wonderful package that you wrote! I’m happy that I could contribute and I hope to make more contributions in the near future as I get to it (e.g., I’ll soon start with integrated problems so #57 is on my radar).

One last thing, should I also rewrite the examples/docs or do you want to do that yourself?

This was referenced May 17, 2022
@N-Wouda
Copy link
Owner

N-Wouda commented May 17, 2022

@leonlan

I make extensive use of ALNS and I’m really grateful for the wonderful package that you wrote! I’m happy that I could contribute and I hope to make more contributions in the near future as I get to it (e.g., I’ll soon start with integrated problems so #57 is on my radar).

Good to hear! I am happy you find it useful. You are more than welcome to have a look at #57 - it should be fairly straightforward to implement. Further, as a fellow user of (and now contributor to!) the library: if there are things you cannot yet do with the current package that you would like to see included, feel free to open an issue to discuss those things further!

One last thing, should I also rewrite the examples/docs or do you want to do that yourself?

I have that on my list, no worries! Master is a bit inconsistent now, but I will fix that with the next release.

@N-Wouda N-Wouda mentioned this pull request May 17, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants