N-Wouda / ALNS

Adaptive large neighbourhood search (and more!) in Python.
https://alns.readthedocs.io/en/latest/
MIT License
442 stars 123 forks source link

Adaptive threshold acceptance #156

Closed TeoSkondras closed 1 year ago

TeoSkondras commented 1 year ago

This PR:

TeoSkondras commented 1 year ago

@N-Wouda @leonlan I would love to receive some feedback!

N-Wouda commented 1 year ago

Hi @TeoSkondras! 👋 Thanks for the PR, I'll try to have a look at this tomorrow!

TeoSkondras commented 1 year ago

Hi @TeoSkondras! 👋 Thanks for the PR, I'll try to have a look at this tomorrow!

Great, I am waiting for your response. I am running the CI on my own repo because I see some style issues.

I added some commits and I think it passes all the CI tests

codecov[bot] commented 1 year ago

Codecov Report

Merging #156 (3c9a4b0) into master (bdcd64b) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   99.44%   99.46%   +0.02%     
==========================================
  Files          29       30       +1     
  Lines         719      746      +27     
==========================================
+ Hits          715      742      +27     
  Misses          4        4              
Impacted Files Coverage Δ
alns/accept/AdaptiveThreshold.py 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

TeoSkondras commented 1 year ago

@N-Wouda I just finished implementing your feedback. The only thing I did not implement is To conform to the AcceptanceCriterion protocol, this method should have the signature you showed me. I cannot understand how to call the method with this signature. If you could explain (alns.tests.states, VarObj etc) I will fix it asap.

N-Wouda commented 1 year ago

@N-Wouda I just finished implementing your feedback. The only thing I did not implement is To conform to the AcceptanceCriterion protocol, this method should have the signature you showed me. I cannot understand how to call the method with this signature. If you could explain (alns.tests.states, VarObj etc) I will fix it asap.

Sure! You could have a look at the other tests (for the other acceptance criteria) to figure out how to call the method? It basically needs a few additional objects that are used by some (not all) other acceptance criteria. Here, you only need candidate, but the others will be passed-in by ALNS regardless.

In those other tests you should see imports like

from alns.tests.states import One, Zero

etc. You can add VarObj to that list of imports: it's the same as your MockCandidate, and should be a drop-in replacement.

TeoSkondras commented 1 year ago

@N-Wouda Thank you for the feedback and the clarifications. I committed my last changes, let me know about anything!

TeoSkondras commented 1 year ago

@N-Wouda Done I think!

N-Wouda commented 1 year ago

I'll merge this in later today. Many thanks for the PR @TeoSkondras!

TeoSkondras commented 1 year ago

@N-Wouda I removed the forgotten print. My pleasure, I am really into OR (mainly VRP-type problems). I would love to exchange contact info with you for other projects!

N-Wouda commented 1 year ago

@TeoSkondras a bit late, my apologies, but if you like VRP-type stuff you might want to have a look at PyVRP as well!