Generate reduce

[SohamTamba]

mapreduce expects other processors to have access to data.
Otherwise, it executes sequentially.

I used @threads instead to run it in parallel.

cc @somil55

[codecov]

Codecov Report

Merging #960 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   99.85%   99.85%   +<.01%     
==========================================
  Files          78       79       +1     
  Lines        2669     2677       +8     
==========================================
+ Hits         2665     2673       +8     
  Misses          4        4

[dsrivastavv]

@SohamTamba This should work for now but how about doing it in a distributed way? You might get a better performance using pmap and then reducing it at the main worker

[SohamTamba]

This issue with pmap is it will use Reps*nv(g) memory.

I tried using @distributed (with 4 workers) but it does not seem to improve performance.

function generate_min_set(g::AbstractGraph{T}, gen_func::Function, Reps::Integer) where T<:Integer

    min_set::Vector{T} = @distributed ((x::Vector{T}, y::Vector{T})->length(x)<length(y) ? x : y) for i in 1:Reps
        gen_func(g)
    end

    return min_set
end

[dsrivastavv]

ok!

[dsrivastavv]

@sbromberger Can you please review it?

[SohamTamba]

I'll make the change in generate_max_set along with the next Hueristic after this and VertexCover is accepted.
I'd prefer to merge as many PR's as possible before GSoC ends.

[SohamTamba]

@somil55
I coded a distributed implementation using remotecall_fetch.

For Ego Twitter graph which has 80,000 vertices, the threaded implementation has the same run time as the distributed implementation when Reps = 1000.

The run-time of the distributed increases because the graph has to be copied in every process.

Benefit of distributed: The code will be more memory efficient since the processes don't share memory.

Disadvantage of distributed: Each process must copy the graph, increasing run-time.

I think the threaded implementation would be better because it seems unlikely for Reps to be more than 100.

Let me know if you want me to PR the distributed implementation.

[dsrivastavv]

@SohamTamba In that case, threaded version seems more practical. What do you think @sbromberger?

[sbromberger]

Why not both with a kwarg in a wrapper function? parallel=: threads or something?

[SohamTamba]

That makes sense.

@threads is more suitable for this algorithm because it performs fast operations while iterating over the graph only once.

remotecall_fetch might be faster for other algorithms. This is the case for centrality measures. The distributed implementation already present is faster than the multi-threaded version I implemented.

[SohamTamba]

@sbromberger
I added a multithreaded and distributed implementation and a wrapper function.

Btw it turns out @distributed implementation is faster than the one with the remote calls.
I just so happen that for Reps = 20, the runtime was 1.2 s and a single sequential call had a runtime of 0.06s.
However, mapeduce did not run in parallel.

[sbromberger]

why is Reps capitalized? Afaik we don't do that anywhere else.

[SohamTamba]

That is the convention I had noticed online.
I can change it to reps instead.

[sbromberger]

Why is Reps capitalized? AFAIK we don't do that anywhere else for variables greater than 1 or 2 chars.

[sbromberger]

Also, with the new Parallel module, this should probably go into Parallel/utils.jl.

[SohamTamba]

I've included it in the parallel module.
Should I replace the implementation using mapreduce from the sequential model?

[SohamTamba]

I was just thinking,
instead of having 2 functions for min_set and max_set, how about I use Base.ordering and create only one function?

[SohamTamba]

On another note, I noticed I tested max_set twice instead of min_set and max_set once each.

Should I create a separate PR to fix it?

[sbromberger]

Looks good but would prefer a variable rename so that all non-struct vars are lower case.

[sbromberger]

This is so minor, but it bothers me: can we make sure standard variables are lower case?

[SohamTamba]

Yeah.
I thought I had done that.

Must be an issue from rebasing.

[sbromberger]

Looks great. Thanks!