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 further engine bindings #6

Merged
merged 6 commits into from
Jul 10, 2023
Merged

Adding further engine bindings #6

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2490fcb
Add further engine support
whytro Jun 30, 2023
171e825
Add __repr__ output for custom JSON types
whytro Jun 30, 2023
c5b00de
Add in MatchParameter support
whytro Jul 3, 2023
af1adc7
Add validity check for TileParameter argument
whytro Jul 3, 2023
0bc51a5
Adjust visitor for JSON types to return string
whytro Jul 3, 2023
cc5ed2b
Adjust MatchParameter arguments and handling
whytro Jul 4, 2023
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
10 changes: 5 additions & 5 deletions src/osrm_nb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ NB_MODULE(osrm_ext, m) {
init_Status(m);

init_BaseParameters(m);
// init_MatchParameters(m);
init_NearestParameters(m);
init_RouteParameters(m);
init_TableParameters(m);
init_TileParameters(m);
init_RouteParameters(m);
init_MatchParameters(m);
init_TripParameters(m);
init_TileParameters(m);

nb::class_<OSRM>(m, "OSRM", nb::is_final())
.def(nb::init<EngineConfig&>())
Expand All @@ -72,8 +72,8 @@ NB_MODULE(osrm_ext, m) {

new (t) OSRM(config);
})
// .def("Match", nb::overload_cast<const MatchParameters&, json::Object&>(&OSRM::Match, nb::const_))
// .def("Match", nb::overload_cast<const MatchParameters&, api::ResultT&>(&OSRM::Match, nb::const_))
.def("Match", nb::overload_cast<const MatchParameters&, json::Object&>(&OSRM::Match, nb::const_))
.def("Match", nb::overload_cast<const MatchParameters&, api::ResultT&>(&OSRM::Match, nb::const_))
.def("Nearest", nb::overload_cast<const NearestParameters&, json::Object&>(&OSRM::Nearest, nb::const_))
.def("Nearest", nb::overload_cast<const NearestParameters&, api::ResultT&>(&OSRM::Nearest, nb::const_))
.def("Route", nb::overload_cast<const RouteParameters&, json::Object&>(&OSRM::Route, nb::const_))
Expand Down
32 changes: 0 additions & 32 deletions src/parameters/matchparameter_nb copy.cpp
View file
Open in desktop

This file was deleted.

83 changes: 83 additions & 0 deletions src/parameters/matchparameter_nb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
#include "parameters/matchparameter_nb.h"

#include "engine/api/match_parameters.hpp"

#include <nanobind/nanobind.h>
#include <nanobind/stl/string.h>
#include <nanobind/stl/vector.h>

namespace nb = nanobind;
using namespace nb::literals;

void init_MatchParameters(nb::module_& m) {
using osrm::engine::api::BaseParameters;
using osrm::engine::api::RouteParameters;
using osrm::engine::api::MatchParameters;

nb::enum_<MatchParameters::GapsType>(m, "MatchGapsType")
.value("Split", MatchParameters::GapsType::Split)
.value("Ignore", MatchParameters::GapsType::Ignore)
.export_values();

nb::class_<MatchParameters, RouteParameters>(m, "MatchParameters")
.def(nb::init<>())
.def("__init__", [](MatchParameters* t,
std::vector<unsigned> timestamps,
MatchParameters::GapsType gaps,
bool tidy,
std::vector<std::size_t> waypoints,
const bool steps,
const bool alternatives,
const RouteParameters::AnnotationsType annotations,
const RouteParameters::GeometriesType geometries,
const RouteParameters::OverviewType overview,
const boost::optional<bool> continue_straight,
std::vector<osrm::util::Coordinate> coordinates,
std::vector<boost::optional<osrm::engine::Hint>> hints,
std::vector<boost::optional<double>> radiuses,
std::vector<boost::optional<osrm::engine::Bearing>> bearings,
std::vector<boost::optional<osrm::engine::Approach>> approaches,
bool generate_hints,
std::vector<std::string> exclude,
const BaseParameters::SnappingType snapping
) {
new (t) MatchParameters(
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've done the MatchParameters in the "assigning" way, but I'm not sure if it would be clearer to just fully assign all values manually, or use the constructor for MatchParameters and RouteParameters, and then manually assign for BaseParameters.

I believe the NodeJS bindings do it in the inverse way instead (ie. assigning BaseParameters, and then everything else is explicitly assigned), but I think that would be more complicated to replicate here.

Copy link
Owner

Choose a reason for hiding this comment

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

but I'm not sure if it would be clearer to just fully assign all values manually

Yeah I agree here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To clarify, do you mean that you agree with manually assigning all values over "hybrid" instantiation?

Copy link
Owner

Choose a reason for hiding this comment

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

you agree with manually assigning all values

exactly that:)

Copy link
Owner

Choose a reason for hiding this comment

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

you want to integrate that change before merging or make an issue out of it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The part that I didn't fully understand about the NodeJS annotations_type part there is that it seems to check what enum type was sent for annotations to assign the annotations_type value (in an array), but for the C++ side of things, it seems to base it off which RouteParameters constructor is called. If I'm understanding this part correctly, the user is able to send in an array of AnnotationsTypes, which gets calculated up? In that case, it would make sense to apply that same abstraction to the Python bindings.

Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side ["nodes", "distance", "speed"] to smth like this on C++ side std::vector<AnnotationType>{AnnotationType::Nodes, AnnotationType::Distance, AnnotationType::Speed}. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The part that I didn't fully understand about the NodeJS annotations_type part there is that it seems to check what enum type was sent for annotations to assign the annotations_type value (in an array), but for the C++ side of things, it seems to base it off which RouteParameters constructor is called. If I'm understanding this part correctly, the user is able to send in an array of AnnotationsTypes, which gets calculated up? In that case, it would make sense to apply that same abstraction to the Python bindings.

Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side ["nodes", "distance", "speed"] to smth like this on C++ side std::vector<AnnotationType>{AnnotationType::Nodes, AnnotationType::Distance, AnnotationType::Speed}. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.

It's definitely possible, but my confusion stemmed from the C++ side of things not accepting a vector of AnnotationsType - only an AnnotationsType enum. On the C++ end, is the user expected to perform that calculation prior to passing it in? Otherwise I can create an abstraction for that, like on the NodeJS bindings

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahhh, I got it. annotations_type is not vector indeed, it is bitmask. Then you indeed can follow NodeJS path and just transform Python's list to this bitmask just like NodeJS bindings do.

So I would rephrase what I said above:

Not sure I fully understand the question... What happens in NodeJS bindings it simply converts something like this on JS side ["nodes", "distance", "speed"] to smth like this on C++ side AnnotationType::Nodes | AnnotationType::Distance | AnnotationType::Speed. Can we do the same on Python side? Ofc we can use https://docs.python.org/3/library/enum.html or smth like this if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, that sounds doable, thanks for the clarifications!

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've made adjustments to accommodate the requested changes. In the process, I've also discovered a bug in osrm-backend's RouteParameter's AnnotationsType |= operator overload that renders it nonfunctional, so I've also put in a PR over there to address that.

timestamps, gaps, tidy,
steps, alternatives, annotations,
geometries, overview, continue_straight
);

t->waypoints = std::move(waypoints);
t->coordinates = std::move(coordinates);
t->hints = std::move(hints);
t->radiuses = std::move(radiuses);
t->bearings = std::move(bearings);
t->approaches = std::move(approaches);
t->generate_hints = generate_hints;
t->exclude = std::move(exclude);
t->snapping = snapping;
},
"timestamps"_a,
"gaps"_a = MatchParameters::GapsType::Split,
"tidy"_a = false,
"waypoints"_a = std::vector<std::size_t>(),
"steps"_a,
"alternatives"_a,
"annotations"_a = RouteParameters::AnnotationsType::None,
"geometries"_a,
"overview"_a,
"continue_straight"_a,
"coordinates"_a = std::vector<osrm::util::Coordinate>(),
"hints"_a = std::vector<boost::optional<osrm::engine::Hint>>(),
"radiuses"_a = std::vector<boost::optional<double>>(),
"bearings"_a = std::vector<boost::optional<osrm::engine::Bearing>>(),
"approaches"_a = std::vector<boost::optional<osrm::engine::Approach>>(),
"generate_hints"_a = true,
"exclude"_a = std::vector<std::string>(),
"snapping"_a = BaseParameters::SnappingType::Default
)
.def_rw("timestamps", &MatchParameters::timestamps)
.def_rw("gaps", &MatchParameters::gaps)
.def_rw("tidy", &MatchParameters::tidy)
.def("IsValid", &MatchParameters::IsValid);
}
4 changes: 2 additions & 2 deletions src/py_osrm/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
TripSourceType,
TripDestinationType,

# MatchParameters,
# MatchGapsType,
MatchParameters,
MatchGapsType,

Object
)