Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Add Karp's minimum cycle mean algorithm #739

Merged
merged 6 commits into from
Sep 6, 2017
Merged

Conversation

blegat
Copy link
Contributor

@blegat blegat commented Sep 4, 2017

Fixes #512

@blegat
Copy link
Contributor Author

blegat commented Sep 6, 2017

I get the following failure

Diffusion Simulation: Test Failed
  Expression: avg > means[p] - (stds[p] / sqrt(runs)) * 3.5
   Evaluated: 0.8999999999999999 > 1.0100505063388334

However, I don't see how the PR have affected this. Is it a known failure ?

@sbromberger
Copy link
Owner

Is it a known failure ?

Yes - it happens occasionally. I'll merge and retest.

Copy link
Owner

@sbromberger sbromberger left a comment

Choose a reason for hiding this comment

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

Thanks, @blegat - this looks pretty cool!

# g::::IsDirected,
# distmx::::AbstractMatrix = weights(g)
# )
function karp_minimum_cycle_mean(g, distmx)
Copy link
Owner

Choose a reason for hiding this comment

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

Make sure distmx has a default:

distmx::AbstractMatrix = weights(g);

(also change the docstring:

"""
    karp_minimum_cycle_mean(g[, distmx])

Return minimum cycle mean of the directed graph `g` with optional edge weights contained in `distmx`.
"""


if iszero(jbest)
return U[], Inf
end
Copy link
Owner

Choose a reason for hiding this comment

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

Short-circuit here:

iszero(jbest) && return U[], Inf

F[1, 1] = 0.
for v in 2:length(component)
F[1, v] = Inf
end
Copy link
Owner

Choose a reason for hiding this comment

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

can you use fill here somehow?

v2j[v] = j
end
n = length(component)
F = Matrix{float(T)}(n+1, n)
Copy link
Owner

Choose a reason for hiding this comment

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

Does it make sense just to make this Float64?

g::AbstractGraph,
distmx::AbstractMatrix{T},
component::Vector{U}
) where T where U<:Integer
Copy link
Owner

Choose a reason for hiding this comment

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

where T <: Real

add_edge!(g, 11, 7)
add_edge!(g, 12, 9)

c, λ = karp_minimum_cycle_mean(g, w)
Copy link
Owner

Choose a reason for hiding this comment

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

make sure you cycle through all graph types: rename g to g1 and do

for g in maketestgraphs(g1)
  c, λ = @inferred(karp_minimum_cycle_mean(g, w))
     @test c == [9, 11, 7]
     @test λ == 0.9
end

(repeat for the other tests as well)

@codecov
Copy link

codecov bot commented Sep 6, 2017

Codecov Report

Merging #739 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #739   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          63     64    +1     
  Lines        3185   3233   +48     
=====================================
+ Hits         3185   3233   +48

@blegat
Copy link
Contributor Author

blegat commented Sep 6, 2017

Thanks for the comments ! I have improved the PR

@sbromberger
Copy link
Owner

Great. I'll do another quick review and would like @jpfairbanks to check it for functionality and things I've missed. :)

Copy link
Contributor

@jpfairbanks jpfairbanks left a comment

Choose a reason for hiding this comment

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

Lgtm

@sbromberger
Copy link
Owner

awesome. Thank you, @blegat and @jpfairbanks !!

@sbromberger sbromberger merged commit b562f01 into sbromberger:master Sep 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants