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

Move broken Parallel.gdistances function to Experimental submodule #94

Conversation

simonschoelly
Copy link
Member

The Parallel.gdistances functions seems to have some incorrect assumptions about threaded memory. This leads to tests failing from time to time. In order to not completely throw away the code, this PR moves it to the Experimental.Parallel submodule and deactivates the tests for this function.

See #10

@codecov
Copy link

codecov bot commented Jan 15, 2022

Codecov Report

Merging #94 (e5b8ffb) into master (abdfceb) will decrease coverage by 1.03%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   98.82%   97.79%   -1.04%     
==========================================
  Files         109      110       +1     
  Lines        6201     6203       +2     
==========================================
- Hits         6128     6066      -62     
- Misses         73      137      +64     

@simonschoelly simonschoelly self-assigned this Jan 15, 2022
Copy link
Member

@etiennedeg etiennedeg left a comment

Choose a reason for hiding this comment

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

There is also gdistances tests in
Graphs.jl/test/traversals/bfs.jl (lines 29-35)

gdistances should not be globally exported, but only exported in Experimental module.

We should also reflect this change in the documentation (gdistances appears in
Graphs.jl/docs/src/pathing.md for example)

period is calling gdistances, what should we do about it ?

@ViralBShah mentionned that he also saw bfs failing, but I couldn't find such failing test.

@ViralBShah
Copy link
Contributor

The bfs failures only happen sporadically in CI for me. I could not reproduce on my own machine.

@simonschoelly
Copy link
Member Author

The tests in Graphs.jl/test/traversals/bfs.jl are for the non-parallel version of gdistances which is is not broken, so it still should be exported:

"""
gdistances(g, source; sort_alg=QuickSort)
Return a vector filled with the geodesic distances of vertices in `g` from
`source`. If `source` is a collection of vertices each element should be unique.
For vertices in disconnected components the default distance is `typemax(T)`.
An optional sorting algorithm may be specified (see Performance section).
### Performance
`gdistances` uses `QuickSort` internally for its default sorting algorithm, since it performs
the best of the algorithms built into Julia Base. However, passing a `RadixSort` (available via
[SortingAlgorithms.jl](https://github.com/JuliaCollections/SortingAlgorithms.jl)) will provide
significant performance improvements on larger graphs.
"""
gdistances(g::AbstractGraph{T}, source; sort_alg = Base.Sort.QuickSort) where T = gdistances!(g, source, fill(typemax(T), nv(g)); sort_alg = sort_alg)

@ViralBShah
Copy link
Contributor

ViralBShah commented Jan 17, 2022

IIRC, it was parallel bfs that was broken - not the sequential one. And it happened only on windows. The logs are all gone now.

@simonschoelly
Copy link
Member Author

The testset in which the failing gdistances test where defined, is called

@testset "Parallel.BFS" begin

Is it possible, that this is the reason why you think that bfs fails from time to time?

@ViralBShah
Copy link
Contributor

Yes that would explain it. I was just going by what I was seeing in the logs.

@etiennedeg
Copy link
Member

Ok, my bad, all my remarks were on sequential gdistances, so I think its ok for me. (gdistances was not even mentionned in the documentation of parallel algorithms)

@simonschoelly
Copy link
Member Author

Thanks for reviewing :) Will merge this now, so hopefully we won't see that many failing tests anymore.

@simonschoelly simonschoelly merged commit 3e4d7ef into JuliaGraphs:master Jan 17, 2022
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.

3 participants