Adding compatability for upstream vroom. #121
Conversation
jonathf
force-pushed
the
serverstring
branch
from
4ecd998 to
fc46276
Compare
src/bind/input/input.cpp
Outdated
| .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) */ |
There was a problem hiding this comment.
@jcoupey, seems like vroom::Input::set_amount_size is removed. Is the feature deprecated or just moved somewhere else?
There was a problem hiding this comment.
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.
src/bind/input/input.cpp
Outdated
| py::arg("nb_searches"), | ||
| py::arg("depth"), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If we can have that, that would be great, yes.
There was a problem hiding this comment.
@jonathf done in VROOM-Project/vroom#1197, let me know if that's OK and I'll merge.
jonathf
force-pushed
the
serverstring
branch
from
1f49b6a to
9a0391e
Compare
jonathf
force-pushed
the
serverstring
branch
3 times, most recently
from
532420d to
f37c360
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
========================================
- Coverage 79.1% 40.3% -38.9%
========================================
Files 29 29
Lines 1628 1630 +2
Branches 138 147 +9
========================================
- Hits 1288 657 -631
- Misses 330 973 +643
+ Partials 10 0 -10
Flags with carried forward coverage won't be shown. Click here to find out more. |
jonathf
force-pushed
the
serverstring
branch
2 times, most recently
from
7fd3db2 to
0b3940e
Compare
jonathf
force-pushed
the
serverstring
branch
3 times, most recently
from
8529802 to
dde63c7
Compare
No description provided.