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

unexpected failing of test_edge_current_flow_betweenness_partition test with windows-latest in nx-parallel #36

Closed
Schefflera-Arboricola opened this issue Jan 22, 2024 · 4 comments · Fixed by networkx/networkx#7251

Comments

@Schefflera-Arboricola
Copy link
Member

Schefflera-Arboricola commented Jan 22, 2024

The test_edge_current_flow_betweenness_partition test is failing with windows-latest in nx-parallel repo, but this is not happening in the main networkx repo. Also, nothing seems wrong with the test. It has been failing for the commits i pushed in different PRs in the last few hours.

PR #35 , #32 , #27

=========================== short test summary info ===========================
FAILED algorithms/community/tests/test_divisive.py::test_edge_current_flow_betweenness_partition - assert {0, 1, 2, 3} in [{3, 4, 5, 6}, {0, 1, 2}]
===== 1 failed, 5260 passed, 57 skipped, 11 warnings in 115.37s (0:01:55) =====
Error: Process completed with exit code 1.
@dschult
Copy link
Member

dschult commented Jan 22, 2024

It looks like there are two valid solutions to the problem presented in the test.
The barbell graph is symmetric. So one solution puts the middle node with the left and the other puts it with the nodes on the right. I haven’t checked this for sure, but it makes sense based on the symmetry of the problem. We don’t see this error in NetworkX because the implementation only finds one of the solutions every time. I’m not sure why the parallel implementation would find a different solution on windows than on the *nix based platforms.

I guess the correct thing to is: 1) verify that there are two solutions. 2) rewrite the test so that it passes with either solution.

answer = [[{0, 1, 2, 3}, {4, 5, 6}], [{0, 1, 2}, {3, 4, 5, 6}]]
assert len(C) == len(answers[0])
assert any(all(s in answer for s in C) for answer in answers)

@Schefflera-Arboricola
Copy link
Member Author

I’m not sure why the parallel implementation would find a different solution on windows than on the *nix based platforms.

It might have something to do with the fact that networkx tests run on "windows" and in nx-parallel we have "windows-latest". So, these 2 OSs might be running the function in different ways.

I tried changing it to "windows" in nx-parallel(Schefflera-Arboricola#1 and #39) but I think it has to be configured at the repository owner side in someway. I don't know much about it, i'm just guessing.

@MridulS
Copy link
Member

MridulS commented Jan 28, 2024

Yes, it does seems to be the OS issue, in https://github.com/networkx/nx-parallel/actions/runs/7656063469/job/20863515922 it passes on windows.

@dschult
Copy link
Member

dschult commented Jan 31, 2024

If there is a different result on windows and windows-latest (and the produced result is actually correct, but not the solution that the test was looking for, then we should probably make the tests check for either of the valid solutions. networkx/networkx#7263 attempts to do this. I would prefer this solution to just changing the OS we are testing under (when we know the other one doesn't pass the tests).

Can you take a look at networkx/networkx#7263? If we merge that then I think these tests will start passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants