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

Test fails with the development version of igraph #232

Closed
krlmlr opened this issue Jan 4, 2023 · 2 comments · Fixed by #235
Closed

Test fails with the development version of igraph #232

krlmlr opened this issue Jan 4, 2023 · 2 comments · Fixed by #235
Labels
bug 🐛 Something isn't working high priority ⏰ Should be fixed soon testing ✅ Related to testing tasks

Comments

@krlmlr
Copy link

krlmlr commented Jan 4, 2023

Describe the bug

One of the tests, https://github.com/luukvdmeer/sfnetworks/blob/main/tests/testthat/test_paths.R#L237-L243, now fails with the development version of igraph. The call distances(mode = "in", algorithm = "johnson") now raises an error.

Distances should be the same, irrespective of the algorithm used by igraph. What's the purpose of this test, can this be achieved in another way (e.g., by mocking)?

Reproducible example

N/A

Expected behavior

Tests pass with the development version of igraph.

R Session Info

Install the latest version of igraph via devtools::install_github("igraph/rigraph@master") .

CC @ntamas @szhorvat.

@szhorvat
Copy link

szhorvat commented Jan 5, 2023

For context on this, please see igraph/rigraph#571. It's not quite settled yet how this will be dealt with in igraph. The original fix restricts the Johnson method to using only mode='out' on directed graphs. The failure here was caused by using mode='in'.

All this said, the test used in sfnetwork does not achieve anything, and I recommend removing or changing it. Johnson's algorithm is intended specifically for directed graphs with some negative edge weights. When there are no negative weights, igraph will just use Dijsktra's algorithm, even if you specified Johnson. Thus the test, in its current form, compares Dijsktra against Dijkstra and can never fail.

Why does igraph just use Dijkstra when there are no negative weights? This makes perfect sense if we look at how Johnson's algorithm works: it simply transforms edge weights to ensure that they are all non-negative, allowing the use of Dijkstra's method instead of the Bellman-Ford algorithm, which has worse asymptotic complexity.

To sum up, I recommend removing the test that @krlmlr referenced, irrespective of how this will be dealt with in igraph 1.3.6. Comparing results between Johnson and Dijkstra makes no sense as the former is only used with negative edge weights, while the latter is only used with non-negative ones.

@loreabad6
Copy link
Collaborator

Hi @krlmlr and @szhorvat, thank you for pointing us to this failing test. The aim of the test is jsut to check that parameters are passed correctly onto igraph::distances(), and during creating this test we probably overlooked the conceptual theory of the algorithms. To keep the test alive, I switched the algorithm to unweighted, which should definitely result in different distances.

@luukvdmeer luukvdmeer added testing ✅ Related to testing tasks bug 🐛 Something isn't working high priority ⏰ Should be fixed soon labels Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working high priority ⏰ Should be fixed soon testing ✅ Related to testing tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants