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

[ALGO] Dijkstra Shortest Path (Tree Version) (#54) #62

Merged
merged 5 commits into from
Aug 6, 2023

Conversation

joweich
Copy link
Collaborator

@joweich joweich commented Aug 6, 2023

Please note:

  • This is not an exact overload of dijkstra_shortest_path as the syntax proposed in [ALGO] Dijkstra Shortest Path (Tree Version) #54 uses the plural paths.
  • There is quite some code duplication between the tree version and the non-tree version. The tree version would be able to cover both with an additional break statement, which is expected to have worse performance though. We could however think of trading some performance for readability and maintainability here.

@joweich joweich requested a review from bobluppes August 6, 2023 15:47
@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (18c5a95) 99.80% compared to head (eba68ed) 99.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   99.80%   99.81%   +0.01%     
==========================================
  Files          22       22              
  Lines        1009     1081      +72     
==========================================
+ Hits         1007     1079      +72     
  Misses          2        2              
Files Changed Coverage Δ
include/graaflib/algorithm/shortest_path.h 100.00% <ø> (ø)
include/graaflib/algorithm/shortest_path.tpp 100.00% <100.00%> (ø)
test/graaflib/algorithm/shortest_path_test.cpp 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Looks excellent, thanks for picking this up so quickly! Just some minor comments.

Regarding the new function not being an overload, you are correct! I slightly prefer this version with the plural paths, but let me know if you think otherwise.

For the potential performance trade-off that you mentioned, my initial feeling is that it would not be too bad. But maybe let's add a benchmark in a follow-up and we can measure the performance impact.

test/graaflib/algorithm/shortest_path_test.cpp Outdated Show resolved Hide resolved
include/graaflib/algorithm/shortest_path.tpp Outdated Show resolved Hide resolved
include/graaflib/algorithm/shortest_path.tpp Outdated Show resolved Hide resolved
include/graaflib/algorithm/shortest_path.tpp Outdated Show resolved Hide resolved
include/graaflib/algorithm/shortest_path.tpp Outdated Show resolved Hide resolved
@bobluppes
Copy link
Owner

Thanks, looks great!

@bobluppes bobluppes merged commit 8280e30 into bobluppes:main Aug 6, 2023
8 checks passed
@bobluppes bobluppes added the enhancement New feature label Aug 6, 2023
@joweich joweich deleted the feature/dijkstra-tree#54 branch August 6, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants