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

Conversation

colin-lewis-19
Copy link
Contributor

New visitors for time & cost thresholds. Setting up the unit tests started to get out of hand, so I've opened this WIP so we can discuss the initial steps

Comment on lines +4 to 8
#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>
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>

@@ -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

Comment on lines +35 to +36
* @details Throws the vertex descriptor that is the termination of the path a valid vertex in the last rung
* is found
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



/**
* @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


/**
* @brief Event visitor that terminates the once a time threshold is met
* @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?



/**
* @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

* is found
*/
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

@@ -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

Comment on lines +40 to +45
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;
}
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;
}

Comment on lines +56 to +59
class timeout_exception : public std::runtime_error {
public:
using std::runtime_error::runtime_error;
};
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

@Levi-Armstrong
Copy link
Collaborator

@marip8, @marrts, and @colin-lewis-19 what is left for this to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants