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

Adds deterministic version for boruvka and dijkstra algorithm. #398

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

fajlip
Copy link
Contributor

@fajlip fajlip commented Feb 6, 2024

The dijkstra() a boruvka() algorithms of CXXGraph (and perhaps others, but we used these two) are non-deterministic; ie they return different results for the same input under some conditions; especially on different platforms (Windows vs Linux).

This is problematic:
1)It breaks unit tests.
2)It introduces non-determinism to the whole program, which is deadly when you want to reproduce some problem (it might just not happen again).

So I added deterministic versions of these two algorithms to CXXGraph:

boruvka_deterministic()
dijkstra_deterministic()
dijkstra_deterministic2()

They work as the original ones, but always return the same result for the same input, not matter the platform.
Well, almost:

dijkstra_deterministic() uses this method of ensuring determinism:
https://stackoverflow.com/questions/69649312/how-to-specify-tie-breaking-logic-in-boost-dijkstra-shortest-paths-dijkstra?noredirect=1&lq=1

But the method can still introduce non-determinism in some very special cases
(namely, two paths have the same length(weights sum) AND the same sum of vertex ids, which is improbable, but can happen).

So I added
dijkstra_deterministic2()
which lexicographically compares the whole paths. This assures 100% determinism; but it is a bit more spatially and temporaly intensive.
So I have kept both versions and let the user choose the version he prefers (99% determinism, or 100% determinism at a cost).

@ghost
Copy link

ghost commented Feb 6, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@ZigRazor
Copy link
Owner

ZigRazor commented Feb 6, 2024

can you add some tests? thank you in advance!

@fajlip
Copy link
Contributor Author

fajlip commented Feb 7, 2024

It could be tested only on different platforms (windows/linux which comes with different versions of STL implementations).
It does not make much sense to test these functions on one platform; as on one platform, the behavior is the same even with the old versions. What the new versions solve is that they give the algorithms the same behavior even on different platforms.
You can use unit tests for normal non-deterministic version of algorithm (you already have those) and it will pass even on new deterministic versions; but I duplicated those unit tests for deterministic algorithms to demonstrate that.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (1b31e63) 97.58% compared to head (0d5b3bb) 97.84%.
Report is 2 commits behind head on master.

Files Patch % Lines
include/CXXGraph/Graph/Algorithm/Dijkstra_impl.hpp 95.15% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   97.58%   97.84%   +0.25%     
==========================================
  Files          87       87              
  Lines        9492    10063     +571     
  Branches        0      657     +657     
==========================================
+ Hits         9263     9846     +583     
+ Misses        229      217      -12     
Flag Coverage Δ
unittests 97.84% <99.24%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@ZigRazor ZigRazor merged commit 1f5ca91 into ZigRazor:master Feb 7, 2024
26 of 33 checks passed
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.

2 participants