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.

Sign up for GitHub

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

Adding compatability for upstream vroom. #121

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Adding compatability for upstream vroom. #121

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
495526c
update upstream vroom
jonathf Nov 16, 2024
fc46276
pushing for 1.15 support
jonathf Nov 23, 2024
d2c95ab
update input interface
jonathf Nov 23, 2024
9a0391e
gcc-13 -> gcc-14
jonathf Nov 23, 2024
2970f62
missing VehicleCost constructor update
jonathf Nov 23, 2024
deb1042
removing amount from input
jonathf Dec 1, 2024
06cf96c
upload-artifact v4
jonathf Dec 1, 2024
b6c24e8
downgrade to v3 again
jonathf Dec 1, 2024
ec593b0
update vroom + version
jonathf Dec 3, 2024
ce64ecf
ambigous solve call fix
jonathf Dec 3, 2024
7ed6e6f
don't verify changes
jonathf Dec 4, 2024
991e9e4
accomodate two versions of sed
jonathf Dec 6, 2024
ab0bca9
bugfix
jonathf Dec 6, 2024
39d4e1f
path in Server init
jonathf Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/_vroom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
#include "structures/typedefs.h"

#include "structures/generic/edge.cpp"
#include "structures/generic/matrix.cpp"
#include "structures/generic/matrix.h"
#include "structures/generic/undirected_graph.cpp"

#include "structures/vroom/cost_wrapper.cpp"
Expand Down
1 change: 0 additions & 1 deletion src/bind/enums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ void init_enums(py::module_ &m) {
py::enum_<vroom::HEURISTIC>(m, "HEURISTIC")
.value("BASIC", vroom::HEURISTIC::BASIC)
.value("DYNAMIC", vroom::HEURISTIC::DYNAMIC)
.value("INIT_ROUTES", vroom::HEURISTIC::INIT_ROUTES)
.export_values();

py::enum_<vroom::INIT>(m, "INIT")
Expand Down
14 changes: 9 additions & 5 deletions src/bind/input/input.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,18 @@ void init_input(py::module_ &m) {

py::class_<vroom::Input>(m, "Input")
.def(
py::init([](const vroom::io::Servers &servers, vroom::ROUTER router) {
return new vroom::Input(servers, router);
py::init([](const vroom::io::Servers &servers, vroom::ROUTER router, bool apply_TSPFix) {
return new vroom::Input(servers, router, apply_TSPFix);
}),
"Class initializer.",
py::arg("servers") = std::map<std::string, vroom::io::Servers>(),
py::arg("router") = vroom::ROUTER::OSRM)
py::arg("router") = vroom::ROUTER::OSRM,
py::arg("apply_TSPFix") = false)
.def_readonly("jobs", &vroom::Input::jobs)
.def_readonly("vehicles", &vroom::Input::vehicles)
.def("_from_json", &vroom::io::parse, py::arg("json_string"),
py::arg("geometry"))
.def("_set_amount_size", &vroom::Input::set_amount_size)
/* .def("_set_amount_size", &vroom::Input::set_amount_size) */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcoupey, seems like vroom::Input::set_amount_size is removed. Is the feature deprecated or just moved somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we still store _amount_size as an optional. It's just that it used to be user-defined which is not really convenient. Now we infer the expected size for amount/capacity arrays directly ourselves in the Input class while adding stuff.

This is more convenient from the user perspective (not having to provide a somewhat redundant info). Also it allows to move some checks upstream in the Input class in the light of VROOM-Project/vroom#1086.

.def("_set_geometry", &vroom::Input::set_geometry)
.def("_add_job", &vroom::Input::add_job)
.def("_add_shipment", &vroom::Input::add_shipment)
Expand Down Expand Up @@ -51,9 +52,12 @@ void init_input(py::module_ &m) {
.def("has_homogeneous_locations",
&vroom::Input::has_homogeneous_locations)
.def("has_homogeneous_profiles", &vroom::Input::has_homogeneous_profiles)
.def("has_homogeneous_costs", &vroom::Input::has_homogeneous_costs)
// .def("vehicle_ok_with_job", &vroom::Input::vehicle_ok_with_job)
.def("_solve", &vroom::Input::solve, "Solve problem.",
py::arg("exploration_level"), py::arg("nb_threads") = 1,
py::arg("nb_searches"),
py::arg("depth"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like vroom::Input and vroom::input::solve have been changed a bit. Do we have documentation for apply_TSPFix, nb_searches and depth? I am guessing the former is experimental, and the two latter are core parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the TSPFix operator is definitely experimental. The reasons for disabling it by default are discussed in VROOM-Project/vroom#1013.

The nb_searches and depth parameters are internal parameters that we don't really expose outside the C++ functions. When using from command-line, there is a "meta" parameter that wraps the various trade-offs we offer -x, --explore arg exploration level to use (0..5) (default: 5). The way this translates to the above is defined in https://github.com/VROOM-Project/vroom/blob/31bace492abfd97155faf91c20b4300dfbfcdcec/src/structures/cl_args.cpp#L78-L90.

Having a simple knob to use on a scale from 0 to 5 is actually convenient from a user perspective because everything is already tuned internally and you don't have to understand what the underlying parameters are about. Maybe that's what we'd also like to expose from pyvroom to offer the same interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think have the parameters consistent would be a good idea, yeah.
For now I'll do the translation manually on the Python side to get something working.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we could do if it helps is to keep an overload of Input::solve with the old exploration_level signature. This would trivially call the new version with the expected conversion for the new parameters. Then you'd only have to keep exposing that one for the bindings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we can have that, that would be great, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonathf done in VROOM-Project/vroom#1197, let me know if that's OK and I'll merge.

py::arg("nb_threads") = 1,
py::arg("timeout") = vroom::Timeout(),
py::arg("h_param") = std::vector<vroom::HeuristicParameters>())
.def("check", &vroom::Input::check);
Expand Down
2 changes: 1 addition & 1 deletion vroom
Open in desktop
Submodule vroom updated 187 files
Loading