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

Add pending delivery checks in PriorityReplace #1179

Conversation

nir-ben-menachem
Copy link

@nir-ben-menachem nir-ben-menachem commented Nov 4, 2024

Issue

Fixes #1162

Tasks

  • Add pending delivery checks to priority replace operator
  • review

@nir-ben-menachem nir-ben-menachem marked this pull request as ready for review November 4, 2024 11:57
@jcoupey
Copy link
Collaborator

jcoupey commented Nov 5, 2024

Thanks for suggesting a fix! We indeed need to use the existing check for pending deliveries as part of the validity check in cvrp::PriorityReplace::is_valid. That will prevent crashes by avoiding picking an invalid move, but we actually need a broader improvement.

The point of the PriorityReplace operator is to try to replace as many tasks at the beginning (resp. end) of a route as possible by a single job, as soon as it improves the priority score. So adding the check will only discard invalid moves, but not account for other possible moves that are both valid and still improving in term of priority.

Illustration: say you have a route starting with P1->D1->P2->D2->P3->... and the first 5 tasks combined have a priority smaller than the one for a single job we could insert as replacement. Then simply adding a check for pending deliveries after P3 will rightly tell that cutting the route right after P3 is invalid. What we need instead is to try replacing a smaller part, e.g. the P1->D1->P2->D2 part with the single job, which is also a priority score improvement.

@nir-ben-menachem
Copy link
Author

Thanks for clarifying, I assume that means the same PR should also add pending delivery checks to fwd_over and bwd_over evaluation in run_ls_step in addition to the current is_valid checks ?

@jcoupey
Copy link
Collaborator

jcoupey commented Nov 6, 2024

Simply adding another check upstream will again only filter out unwanted moves, not help reaching out to other valid ones.

The current fwd_last_rank value as passed to the PriorityReplace ctor is the one we'd like to use if valid wrt pending deliveries, but if not we should reach out for the highest possible value below fwd_last_rank that is OK for the pending check (if any). So essentially I think once we found the current fwd_last_rank candidate we need to add a loop back with new checks. Same symmetric situation with bwd_first_rank.

This is starting to be a lot of boilerplate prior to calling the PriorityReplace ctor so I wonder if it wouldn't be better to move the responsibility of determining the ranks to the operator itself.

@nir-ben-menachem
Copy link
Author

After Discussing with @jcoupey, a more comprehensive fix is in the works

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.

Invalid route reached with PriorityReplace
2 participants