Enhancement: Catch negative edge weights in dijkstra #67
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files
☔ View full report in Codecov by Sentry. |
| if (edge_weight < 0) { | ||
| throw std::invalid_argument{fmt::format( | ||
| "Negative edge weight [{}] between vertices [{}] -> [{}].", | ||
| edge_weight, current.id, neighbor)}; | ||
| } |
There was a problem hiding this comment.
[sidenote]: I know it is sub-optimal, but I would like to restrict the usage of fmt to just the test and example source code. That way we can keep the library independent of other libraries, such that clients can install it header-only.
Last time I checked GCC and Clang still had some issues with the std::format implementation, so until then we will have to resort to manual string concatenation 😅
| if (edge_weight < 0) { | |
| throw std::invalid_argument{fmt::format( | |
| "Negative edge weight [{}] between vertices [{}] -> [{}].", | |
| edge_weight, current.id, neighbor)}; | |
| } | |
| if (edge_weight < 0) { | |
| throw std::invalid_argument{ | |
| "Negative edge weight [" + edge_weight + "] between vertices [" + current.id + "] -> [" + neighbor + "]."}; | |
| } |
There might be some other places left in the codebase, so I can also go over this in a follow up.
| template <typename T> | ||
| struct DijkstraShortestPathSignedTypesTest : public testing::Test { | ||
| using graph_t = typename T::first_type; | ||
| using edge_t = typename T::second_type; | ||
| }; | ||
|
|
||
| using weighted_graph_signed_types = testing::Types< |
There was a problem hiding this comment.
I don't see a much better solution than this 👍🏻
In a follow-up I plan to rethink our testing code and see if I can extract some common patterns. For instance I think it would be sufficient to have 2 or 3 such test fixture definitions which we can reuse through the entire testing code.
Furthermore it might be nice to have some common pre-defined test structures we ca re-use for testing the various algorithms.
If you have any more ideas/pain points I would be happy to hear them!
Dijkstra's algorithm can't deal with negative edge weighs. Until now, we don't check this, which could lead to unexpected or misleading results. This PR fixes this by throwing an error on negative weights during the traversal.
The additional tests introduce some boilerplate code since we need to add a new type set that is purely signed. I'm open for feedback regarding more elegant ways to solve this!