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

Jump to bottom

Implement waypoints parameter in match plugin #4732

Merged
merged 1 commit into from
Jan 8, 2018
Merged

Implement waypoints parameter in match plugin #4732

merged 1 commit into from
Jan 8, 2018

Conversation

karenzshea
Copy link
Contributor

@karenzshea karenzshea commented Dec 11, 2017 •
edited
Loading

Issue

Closes #4669

This adds a parameter to the match service plugin, &waypoints, that will allow users to specify which input coordinates should be treated as waypoints in the response. Current behavior is to treat all input coordinates as waypoints. Implementation details here

Tasklist

  • Write unit tests for parameter
    • CollapseInternalRouteResult
    • valid map matching parameters
    • invalid map matching parameters
    • integration tests map matching
  • Add support for map matching results in cucumber support code
  • Debug why waypoints crashes when trace splitting happens Don't support trace splitting and waypoints together
  • Add waypoints handling into tracepoint generation
  • Add parameter parsing to match plugin interface
  • Add handling into trace tidying
  • CHANGELOG.md entry (How to write a changelog entry)
  • write documentation
  • review
  • adjust for comments

Requirements / Relations

nerp

@karenzshea karenzshea force-pushed the feature/stepmaster branch 2 times, most recently from d3e3d52 to 506d774 Compare December 14, 2017 14:40
TheMarex
TheMarex requested changes Dec 19, 2017
View reviewed changes
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

There are some algorithmic things we can simplify. I can run this out, but I'm adding the comments here for visibility to you.

include/engine/api/match_parameters.hpp Outdated
{
}

template <typename... Args>
MatchParameters(std::vector<unsigned> timestamps_, GapsType gaps_, bool tidy_, Args... args_)
MatchParameters(std::vector<unsigned> timestamps_,
Copy link
Member

Choose a reason for hiding this comment

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

We need to preserve the original constructor here to preserve backwards compatibility.

include/engine/api/match_parameters.hpp Outdated

bool IsValid() const
{
auto invalid_idx = std::find_if(waypoints.begin(), waypoints.end(), [this](const auto &w) {
Copy link
Member

Choose a reason for hiding this comment

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

We can use std:all_of instead.

include/engine/api/match_parameters_tidy.hpp Outdated
if (result.was_waypoint[i] && (result.parameters.waypoints.back() != last_idx))
{
result.parameters.waypoints.push_back(last_idx);
// result.was_waypoint[i] = false;
Copy link
Member

Choose a reason for hiding this comment

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

Commented out code should be removed.

include/engine/api/match_parameters_tidy.hpp Outdated
// todo: this is not always true... e.g. if the first or last coordinate is duplicated and then
// tidied away
// but we do want some kind of check like this
// BOOST_ASSERT(result.parameters.waypoints.size() == params.waypoints.size());
Copy link
Member

Choose a reason for hiding this comment

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

Hrm I don't understand this comment or the assertion. Can we specify this more precisely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I actually don't think this needs to be assertion anymore, since it's not always true. I think having test coverage over this is good enough for now.

include/engine/internal_route_result.hpp Outdated

InternalRouteResult collapsed;
collapsed.shortest_path_weight = leggy_result.shortest_path_weight;
for (std::size_t i = 0; i < leggy_result.unpacked_path_segments.size(); i++)
Copy link
Member

Choose a reason for hiding this comment

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

We can use util::irange<size_t> here.

include/engine/internal_route_result.hpp
// case)
if (!last_segment.empty())
last_segment.pop_back();
// update target phantom node of leg
Copy link
Member

Choose a reason for hiding this comment

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

Needs an assertion that segment_end_coordinates is not empty.

src/engine/plugins/match.cpp Outdated
if (index != sm.indices.end())
found = true;
});
if (!found)
Copy link
Member

Choose a reason for hiding this comment

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

Needs brackets around body.

src/engine/plugins/match.cpp Outdated
}

// Error: Check if user-supplied waypoints can be found in the resulting matches
for (const auto waypoint : tidied.parameters.waypoints)
Copy link
Member

@TheMarex TheMarex Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The complexity of this check is too high (three nested loops). What about using a std::set<std::size_t> that contains the waypoint indices. Then we could loop over all sub-matching indices and remove the index of the waypoint we found.

src/engine/plugins/match.cpp Outdated
{
std::vector<bool> waypoint_legs;
waypoint_legs.reserve(sub_matchings[index].nodes.size());
for (unsigned i = 0; i < sub_matchings[index].nodes.size(); ++i)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do this in a single sweep over the indices of the submatch:

  • Allocate a vector of size #waypoints
  • Iterate over indices and parameters.waypoints at the same time (they are both sorted increasing)
  • Set waypoints_legs[i] to true if we find a match.

@karenzshea karenzshea force-pushed the feature/stepmaster branch 2 times, most recently from f54c5a4 to 616e620 Compare January 2, 2018 14:47
oxidase
oxidase approved these changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

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

LGTM!

TheMarex
TheMarex approved these changes Jan 5, 2018
View reviewed changes
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

Looks good, just one nit pick.

src/engine/plugins/match.cpp Outdated
{
std::set<std::size_t> tidied_waypoints(tidied.parameters.waypoints.begin(),
tidied.parameters.waypoints.end());
std::for_each(sub_matchings.begin(), sub_matchings.end(), [&](const SubMatching &sm) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of for_each you can use for (const auto& sm : sub_matchings) { for (auto index : sm.indices) {} } which looks a little cleaner.

@karenzshea karenzshea merged commit 55cc06f into master Jan 8, 2018
@karenzshea karenzshea deleted the feature/stepmaster branch January 8, 2018 11:28
This was referenced Jan 20, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheMarex TheMarex TheMarex approved these changes

@oxidase oxidase oxidase approved these changes

Assignees
No one assigned
Labels
Review - In feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@karenzshea @TheMarex @oxidase