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

Adaptive threshold acceptance #156

Merged
merged 13 commits into from
Jun 23, 2023

Conversation

TeoSkondras
Copy link
Contributor

@TeoSkondras TeoSkondras commented Jun 20, 2023

This PR:

@TeoSkondras
Copy link
Contributor Author

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

@N-Wouda N-Wouda self-requested a review June 20, 2023 20:59
@N-Wouda
Copy link
Owner

N-Wouda commented Jun 20, 2023

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

@TeoSkondras
Copy link
Contributor Author

TeoSkondras commented Jun 21, 2023

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
Copy link

codecov bot commented Jun 21, 2023

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%> (ø)

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

Copy link
Owner

@N-Wouda N-Wouda left a comment

Choose a reason for hiding this comment

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

Looks very good already, thanks! I have a few small remarks that should hopefully not take a lot of time to implement. Thanks again for the PR!

alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
@TeoSkondras
Copy link
Contributor Author

TeoSkondras commented Jun 21, 2023

@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 N-Wouda self-requested a review June 21, 2023 15:43
@N-Wouda
Copy link
Owner

N-Wouda commented Jun 21, 2023

@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
Copy link
Contributor Author

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

Copy link
Owner

@N-Wouda N-Wouda left a comment

Choose a reason for hiding this comment

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

I think these are the last changes I have. Other than that we're nearly good to go. I will have another look at the documentation later, but that might also be after merging!

alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/tests/test_adaptive_threshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
@TeoSkondras
Copy link
Contributor Author

@N-Wouda Done I think!

Copy link
Owner

@N-Wouda N-Wouda left a comment

Choose a reason for hiding this comment

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

Can you remove the print statement? Otherwise looks good to me! Thanks for the PR! 🎉

alns/accept/AdaptiveThreshold.py Outdated Show resolved Hide resolved
@N-Wouda
Copy link
Owner

N-Wouda commented Jun 23, 2023

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

@TeoSkondras
Copy link
Contributor Author

TeoSkondras commented Jun 23, 2023

@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 N-Wouda merged commit d2b3871 into N-Wouda:master Jun 23, 2023
8 checks passed
@N-Wouda
Copy link
Owner

N-Wouda commented Jul 26, 2023

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

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.

Adaptive threshold acceptance criterion
2 participants