added in forest fire graph generator and needed helper functions #71
Conversation
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 92.38% 92.68% +0.30%
==========================================
Files 21 22 +1
Lines 2126 2228 +102
==========================================
+ Hits 1964 2065 +101
- Misses 162 163 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/generators.jl
Outdated
| seed::Union{Nothing,UInt}=nothing, | ||
| random_node=x->rand(1:x)) where S | ||
|
|
||
| if !(seed === nothing) |
There was a problem hiding this comment.
Can you replace this with specifying the RNG where Random.GlobalRNG is the default? Then they can give StableRNG(seed) if they want reliable seeds...
There was a problem hiding this comment.
GIven that we let the user pass in the random_node function as well, how should I adapt it to work with an rng variable?
Seems like random_node=(x,rng)->rand(rng,1:x)) would force the user to think about a seed if they want to adjust the parent's probability of being selected.
There was a problem hiding this comment.
The convention is for rand functions in Julia to take in an optional random number generator to avoid using the global one. Users should have to think about how to make their random results reproducible if they want them :), but it should be easy enough ... The random node stuff is just a placeholder right now, let's not worry about it.
src/generators.jl
Outdated
|
|
||
| end | ||
|
|
||
| A = list_of_list_to_sparse_matrix(neighbors) |
There was a problem hiding this comment.
Can you use the parametric types to handle this?
e.g. _convert_to_output(input, sparse) which will convert sparse to input type?
src/generators.jl
Outdated
| end | ||
|
|
||
|
|
||
| function _burn!(neighbors::Vector{Vector{S}},new_v::Int,v0::Int,burn_p::Float64, |
There was a problem hiding this comment.
IF this is easy --- can we make this a top level function? e.g. there are other instances where you want to do forest fire sampling... Since the process is the same, it'd be nice to be able to reuse the function.
There was a problem hiding this comment.
What's the best interface for this to be a top function? burn(A,v0::Int,p::Float64) for just a single step and return final neighbor list of the random walk? Or something like burn(A,steps::Int, rand_v::Int->Int) which runs the process for a number of steps and selects random nodes by calling rand_v().
There was a problem hiding this comment.
burn(A,v0,p) and burn!(A,v0,p,temparrays...) are the best top level interface
src/network_formatting.jl
Outdated
| matrix_to_list_of_list(A::MatrixNetwork) = matrix_to_list_of_list(eltype(A.ci),A) | ||
|
|
||
|
|
||
| function matrix_to_list_of_list(::Type{S}, A::MatrixNetwork) where S |
There was a problem hiding this comment.
Can you write these in terms of getting the rowptr/colval/etc.
There was a problem hiding this comment.
These is the matrix_to_list_of_lists
src/network_formatting.jl
Outdated
| @@ -0,0 +1,43 @@ | |||
| matrix_to_list_of_list(A::SparseMatrixCSC) = matrix_to_list_of_list(eltype(A.rowval),A) | |||
|
|
|||
| function matrix_to_list_of_list(::Type{S}, A::SparseMatrixCSC) where S | |||
There was a problem hiding this comment.
There is a subtle bug here, this is correct for undirected graphs, but not for directed graphs since that will transpose in-edges to out-edges.
There was a problem hiding this comment.
Not sure what the best solution for this is. When I wrote them it was intended for undirected graphs because the
forest_fire_graph is expecting an undirected input, and generates new edges with the intention of symmetrizing them.
I can either
- Rename
list_of_list_to_matrixto reference they’re for undirected neighbor lists - Write a
_get_inedgesfor MatrixNetworks to be more careful, but we’d also need to rename thelist_of_list_to_matrixfunction to better express what the output is, because it won’t be an undirected neighbor list. - Convert MatrixNetwork to a SparseCSCMatrix before calling the code, but this seems like a bad solution as the package is about the MatrixNetwork.
There was a problem hiding this comment.
The standard solution in MatrixNetworks is just to take a transpose... if they want to avoid that, then they use a MatrixNetwork type instead.
There was a problem hiding this comment.
ah, I was thinking about the wrong function! Ok that works
| end | ||
|
|
||
|
|
||
| function _burn!(neighbors::Vector{Vector{S}},new_v::Int,v0::Int,burn_p::Float64, | ||
| toburn,burnt,burning,alive) where S | ||
| """ |
There was a problem hiding this comment.
Sorry if there was any misunderstanding, burn should not add the edges to the graph. It should simply return the the set of burned vertices. e.g. if you wanted to run a forest fire sample of an existing graph, you want to run "burn" from multiple vertices. This is a sampled graph that should have similar properties to the original. But you definitely don't want to add anything.
There was a problem hiding this comment.
Taken care of! Any suggestions on tests that can be added to this routine? Currently just ensuring seeded calls return the same for MatrixNetwork and SparseMatrixCSC inputs.
src/generators.jl
Outdated
| (i >= 1 && i <= size(A,1)) || throw(ArgumentError("i must be in {1,...,size(A,1)}")) | ||
| return A.ci[A.rp[i]:A.rp[i+1]-1],A.vals[A.rp[i]:A.rp[i+1]-1] | ||
| @boundscheck (i >= 1 && i <= size(A,1)) || throw(ArgumentError("i must be in {1,...,$(size(A,1))}")) | ||
| return A.ci[A.rp[i]:A.rp[i+1]-1],A.vals[A.rp[i]:A.rp[i+1]-1] |
There was a problem hiding this comment.
Do you need to return an array? can you return a generator or a view?
There was a problem hiding this comment.
They're now returning views, and matrix_to_list_of_list uses collect to allocate the arrays needed.
…erators now, and small documentation changes.
No description provided.