Improve arg syntax for gplot and cleanup repo
#180
Conversation
-distinguish args, kwargs -add missing kwargs
clarify that Cairo is required to visualize in vscode
Checking for System OS was outdated
Now matches gadfly.jl (https://github.com/GiovineItalia/Gadfly.jl/blob/master/src/open_file.jl)
-allow gplothtml to accept same arguments as gplot -allow x/y-locs to be of different type (<: Real)
and minor changes to plots.jl update README
-breaking change: linetype changed from String to Symbol -breaking change: default for edgelabel is nothing (not []) to make consistent with nodelabel default -remove all caps args -improve some of the code in gplot (use isnothing, etc.)
Codecov Report
@@ Coverage Diff @@
## master #180 +/- ##
==========================================
+ Coverage 32.92% 36.73% +3.81%
==========================================
Files 9 4 -5
Lines 565 471 -94
==========================================
- Hits 186 173 -13
+ Misses 379 298 -81
Help us with your feedback. Take ten seconds to tell us how you rate us. |
fix background color
-use `iszero` -remove `stroke(nothing)` or `fill(nothing` as `nothing` is already the default for these functions in Compose.jl
-move layouts to layouts.jl -add community_layout to readme -remove deprecations -remove shape.jl and pienode.jl (not used anywhere)
hdavid16
changed the title
gplotgplot and cleanup repo
Multiple dispatch was messing up because types were not specified for spring layout and `kws` not preceeded by `;`
| @@ -284,3 +284,288 @@ function _spectral(A::SparseMatrixCSC) | |||
| index = sortperm(real(eigenvalues))[2:3] | |||
| return real(eigenvectors[:, index[1]]), real(eigenvectors[:, index[2]]) | |||
| end | |||
|
|
|||
| # This layout algorithm is copy from [IainNZ](https://github.com/IainNZ)'s [GraphLayout.jl](https://github.com/IainNZ/GraphLayout.jl) | |||
There was a problem hiding this comment.
Is there any advantage in moving this function? Maybe it would be easier to read if we keep it in a separate file.
There was a problem hiding this comment.
I figured that since we had a layout.jl file with several layouts, I'd just add it there. However, we could have a layout folder and have a separate file for each layout. What are your thoughts?
src/plot.jl
Outdated
| end | ||
| linetype = :straight, | ||
| outangle = π / 5, | ||
| backgroundc = nothing) where {T <:Integer, R1 <: Real, R2 <: Real} |
There was a problem hiding this comment.
I think it would be easier to review/write tests, if new features where in a separte PR
There was a problem hiding this comment.
Yes. I put it in a PR, but then built further from that one...oops. I'll wait for the background color PR to get merged.
There was a problem hiding this comment.
removed since this is done in a separate PR
| """ | ||
| function gplot(g::AbstractGraph{T}, | ||
| locs_x_in::Vector{R1}, locs_y_in::Vector{R2}; | ||
| nodelabel = nothing, | ||
| nodelabelc = colorant"black", | ||
| nodelabelsize = 1.0, | ||
| NODELABELSIZE = 4.0, | ||
| max_nodelabelsize = 4.0, |
There was a problem hiding this comment.
Totally agree, that we should change this, but I think we should add deprecation warnings for the older functions (that we can eventually remove), or at least produce a meaningful error message.
| outangle = π / 5, | ||
| backgroundc = nothing) where {T <:Integer, R1 <: Real, R2 <: Real} | ||
|
|
||
| @assert length(locs_x_in) == length(locs_y_in) == nv(g) "Position vectors must be of the same length as the number of nodes" |
There was a problem hiding this comment.
We should only use @assert in situations that we expect that they are always true, as it might be removed by a compiler optimization: https://docs.julialang.org/en/v1/base/base/#Base.@assert
There was a problem hiding this comment.
I see. I'll use the if -> error form for these.
| if max_nodestrokelw > 0.0 | ||
| max_nodestrokelw = EDGELINEWIDTH / max_nodestrokelw | ||
| nodestrokelw *= max_nodestrokelw | ||
| if !iszero(max_nodestrokelw) |
There was a problem hiding this comment.
Would testing for max_nodestrokelw not make more sense in case that max_nodestrokelw is negative?
There was a problem hiding this comment.
We are just catching for divide by 0 errors here I think. Passing negative values should work, they just don't get displayed. However, we can check for all inputs being positive when we do the other checks at the beginning of the function if that would be better.
| if !isempty(edgelabel) && length(edgelabel) != NE | ||
| error("Must have one label per edge (or none)") | ||
| end | ||
| linetype = :straight, |
There was a problem hiding this comment.
Also here we should add a deprecation warning, if one uses a String
|
This PR also fixes #148 |
Remove commits that are in separate PRs This reverts commit d4a2ae4ba42a701ebe49865d2addf212c54f7bd7.
Remove commits that are in separate PRs This reverts commit d4a2ae4ba42a701ebe49865d2addf212c54f7bd7.
-breaking change: linetype changed from String to Symbol
-breaking change: default for edgelabel is nothing (not []) to make consistent with nodelabel default
-remove all caps args (Fix #177)
-improve some of the code in gplot (use isnothing, etc.)
-move layouts to layouts.jl
-add community_layout to readme
-remove deprecations
-remove shape.jl and pienode.jl (not used anywhere)