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

Bellman-Ford without negative weight cycle detection #68

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

ndcroos
Copy link
Contributor

@ndcroos ndcroos commented Aug 12, 2023

See #55
This PR attempts to covers all subtasks, except test coverage of at least 95%.
Documentation is a short description for now.
Test cases are now taken from those for Dijkstra's shortest paths method.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi there! Thank you for creating your first pull-request on the Graaf library :)

@ndcroos ndcroos changed the title Bellman-Ford Bellman-Ford without negative weight cycle detection Aug 12, 2023
@bobluppes
Copy link
Owner

Hi @ndcroos, thank you for your contribution!

Looking at the CI, it seems the clang-format and test workflows are failing. The format workflow could be fixed by running clang-format on your branch (more details on the wiki).

For the tests, it seems that some of the new tests either have a failing assert or a segfault. Maybe you could already take a look at this.

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 very good already, thanks for working on this!

Some minor comments, and an issue with the handling of the edge weights. I just checked locally and I think adjusting the weight handling should fix the failing tests.

cmake-modules Outdated Show resolved Hide resolved
docs/docs/algorithms/shortest-path/bellman_ford.md Outdated Show resolved Hide resolved
docs/docs/algorithms/shortest-path/bellman_ford.md 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
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Patch coverage: 98.52% and project coverage change: -0.07% ⚠️

Comparison is base (b8bd54e) 99.74% compared to head (d8029a2) 99.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
- Coverage   99.74%   99.67%   -0.07%     
==========================================
  Files          23       23              
  Lines        1158     1226      +68     
==========================================
+ Hits         1155     1222      +67     
- Misses          3        4       +1     
Files Changed Coverage Δ
include/graaflib/algorithm/shortest_path.h 100.00% <ø> (ø)
include/graaflib/algorithm/shortest_path.tpp 98.90% <94.73%> (-1.10%) ⬇️
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 great, thanks for contributing 🚀
I created #76 to keep track of the negative weight cycle detection.

@bobluppes bobluppes merged commit 8dbe05f into bobluppes:main Aug 14, 2023
8 checks passed
@bobluppes bobluppes added the enhancement New feature label Aug 14, 2023
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