-
Notifications
You must be signed in to change notification settings - Fork 39
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
Heterogeneous vehicle type #245
Conversation
* Remove angleCenter from route * Remove sorting of routes in LocalSearch::exportIndividual
* Take into account route assignment when comparing individuals * Add tests for heterogeneous capacities
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
…yVRP into heterogeneous-capacity
Co-authored-by: Leon Lan <30997278+leonlan@users.noreply.github.com>
* Rename num_vehicles to max_num_routes * Rename num_routes to num_non_empty_routes
Summarizing, I think there are a few leftover issues which I think should be non-blocking. Can we finish this PR and put this in a seperate issue? These I think we should definitely address:
These I am happy with the current situation but we could change if you feel strongly
|
Agree, the first two are most important. I proposed above that we revert our printing to current main. I also want to get rid of
I think the current implementation is fine and much simpler than the initial one.
I want to get rid of this. A client has neighbours, sure, but how is a client assigned a vehicle type? A client is part of a
No is OK, but I'm really not sure why this was in the scope of this PR in the first place.
I'd like to go back to the original, but this isn't worth the discussion to me.
Removed this in the other PR. |
@N-Wouda thanks for the contributions, feel free to propose an alternative for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to commit a few more changes after reading this again, but I'm OK with this. Let's copy the reading stuff over to a new issue and merge this after the benchmark completes OK.
Benchmark should be in tomorrow morning.
Slightly better than #240, which was the last benchmark I could find. Good! |
This PR:
Benchmark
On VRPTW.
This PR (c25917c; job ID 2670926; 10x seeds/1h runtime with
configs/vrptw.toml
):Notes:
This keeps the code coverage level up, and helps ensure the changes work as intended.