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.

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

WIP: new visitors for cost & time #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 49 additions & 1 deletion descartes_light/bgl/include/descartes_light/bgl/event_visitors.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#ifndef DESCARTES_LIGHT_SOLVERS_BGL_EVENT_VISITORS
#define DESCARTES_LIGHT_SOLVERS_BGL_EVENT_VISITORS

#include <chrono>
#include <descartes_light/bgl/boost_graph_types.h>

#include <descartes_light/descartes_macros.h>
DESCARTES_IGNORE_WARNINGS_PUSH
#include <boost/graph/visitors.hpp>
Comment on lines +4 to 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put includes from outside this project in between the ignore warnings macro

- #include <chrono>
#include <descartes_light/bgl/boost_graph_types.h>

#include <descartes_light/descartes_macros.h>
DESCARTES_IGNORE_WARNINGS_PUSH
#include <boost/graph/visitors.hpp>
+#include <chrono>

Expand All @@ -29,6 +29,54 @@ struct early_terminator : public boost::base_visitor<early_terminator<EventType>
const long last_rung_idx_;
};

/**
* @brief Event visitor that terminates the search when a vertex in the last rung of the graph is found
* to be below the provided cost threshold
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* to be below the provided cost threshold
* to be below the provided distance threshold

* @details Throws the vertex descriptor that is the termination of the path a valid vertex in the last rung
* is found
Comment on lines +35 to +36
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is only relevant for non-optimal searches like DFS, right? I would make note of that here

*/
template <typename FloatType, typename EventType>
struct distance_terminator : public boost::base_visitor<distance_terminator<FloatType, EventType>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there value in keeping this separate from the early_terminator? I could see these being combined where the distance_threshold has a default value of max float

{
/** @brief Event filter typedef defining the events for which this visitor can be used */
typedef EventType event_filter;

distance_terminator(long last_rung_idx, FloatType distance_threshold);

void operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g);

const FloatType distance_threshold_;
const long last_rung_idx_;
};


/**
* @brief The timeout_exception class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the description

*/
class timeout_exception : public std::runtime_error {
public:
using std::runtime_error::runtime_error;
};
Comment on lines +56 to +59
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you use the default constructor of runtime_error you'll have to construct this exception with a string message which may not make sense. If you don't want to do this, you might be better off inheriting directly from std::exception



/**
* @brief Event visitor that terminates the once a time threshold is met
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @brief Event visitor that terminates the once a time threshold is met
* @brief Event visitor that terminates the search after a time threshold has been exceeded

* @details How will this throw an end descriptor
Copy link
Collaborator

Choose a reason for hiding this comment

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

How indeed?

*/
template <typename FloatType, typename EventType>
struct time_terminator : public boost::base_visitor<time_terminator<FloatType, EventType>>
{
/** @brief Event filter typedef defining the events for which this visitor can be used */
typedef EventType event_filter;

time_terminator(double time_threshold);

void operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might consider making this a template function as well so it can be used on events with edges or vertices (assuming you don't specifically need an edge or vertex to do your operation)

Suggested change
void operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g);
template<typename T>
void operator()(T u, const BGLGraph<FloatType>& g);


const std::chrono::time_point<std::chrono::steady_clock> start_time_;
const double time_threshold_;
};

/**
* @brief Event visitor that adds all edges to each vertex dynamically as the vertex is discovered during the graph
* search
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,21 @@ static VertexDesc<FloatType> solveDFS(BGLGraph<FloatType>& graph,
if (target != ladder_rungs.back().end() && graph[*target].distance < std::numeric_limits<FloatType>::max())
throw *target;
}
catch(const descartes_light::timeout_exception& te)
{
// If this has to be called in two places, make it a function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I would be opposed to adding this as a helper function to BGLSolverBase since it's used multiple times here and in the Dijkstra search

auto target = std::min_element(ladder_rungs.back().begin(),
ladder_rungs.back().end(),
[&](const VertexDesc<FloatType>& a, const VertexDesc<FloatType>& b) {
return graph[a].distance < graph[b].distance;
});

// Check that the identified lowest cost vertex is valid and has a cost less than inf
if (target != ladder_rungs.back().end() && graph[*target].distance < std::numeric_limits<FloatType>::max())
throw *target;

}

catch (const VertexDesc<FloatType>& target)
{
return target;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,33 @@ void early_terminator<EventType>::operator()(VertexDesc<FloatType> u, const BGLG
throw u;
}

template <typename FloatType, typename EventType>
distance_terminator<FloatType, EventType>::distance_terminator(long last_rung_idx, FloatType distance_threshold) : distance_threshold_(distance_threshold),
last_rung_idx_(last_rung_idx)
{
}

template <typename FloatType,typename EventType>
void distance_terminator<FloatType, EventType>::operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g)
{
if (g[u].rung_idx == last_rung_idx_ && g[u].distance < distance_threshold_)
throw u;
}

template <typename FloatType, typename EventType>
time_terminator<FloatType, EventType>::time_terminator(double time_threshold) : time_threshold_(time_threshold)
{
start_time_ = std::chrono::steady_clock::now();
}

template <typename FloatType, typename EventType>
void time_terminator<FloatType, EventType>::operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g)
{
//if (time_threshold_ < std::chrono::duration<double>(std::chrono::steady_clock::now() - start_time_).count())
throw false;
}
Comment on lines +40 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
template <typename FloatType, typename EventType>
void time_terminator<FloatType, EventType>::operator()(VertexDesc<FloatType> u, const BGLGraph<FloatType>& g)
{
//if (time_threshold_ < std::chrono::duration<double>(std::chrono::steady_clock::now() - start_time_).count())
throw false;
}
template <typename FloatType, typename EventType>
template<typename T>
void time_terminator<FloatType, EventType>::operator()(T, const BGLGraph<FloatType>&)
{
if (time_threshold_ < std::chrono::duration<double>(std::chrono::steady_clock::now() - start_time_).count())
throw timeout_exception;
}



template <typename FloatType, typename EventType>
add_all_edges_dynamically<FloatType, EventType>::add_all_edges_dynamically(
std::vector<typename EdgeEvaluator<FloatType>::ConstPtr> edge_eval,
Expand Down