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

Added optional keyword parameter to gplot for plotting Chain Graphs #110

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dietercastel
Copy link

@dietercastel dietercastel commented May 16, 2020

Hi all,

I added support for "mixed model" graphs or so called "chain graphs" with an optional keyword parameter called "chainGraph". It (selectively) removes the double arrows for bi-directional edges for SimpleDiGraphs.

Visual Examples below:

g = SimpleDiGraph([ 0 1 0 1 ; 1 0 1 0 ; 1 0 0 0 ; 1 0 1 0])
gplot(d, chainGraph=true)

gives
chainGraphTrue

d = SimpleDiGraph([ 0 1 0 0 0; 0 0 1 1 1; 0 1 0 1 1; 0 1 1 0 1; 0 1 1 1 0])
gplot(d, chainGraph=true, nodesize = [0.6; 1.0; 1.0; 1.0; 1.0])

gives
chainGraphSizeTrue

I also added a (visual) test set for that case. I haven't implemented it for the linestyle="curve" option though. Should an error be thrown in that case (until it's implemented)?

I'm also wondering about the placement of the helper functions hasReverseEdge and filterUndef currently they reside in lines.jl where they are used.

kr,
Dieter

@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #110 into master will increase coverage by 5.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   32.02%   37.38%   +5.35%     
==========================================
  Files           9        9              
  Lines         509      527      +18     
==========================================
+ Hits          163      197      +34     
+ Misses        346      330      -16     
Impacted Files Coverage Δ
src/lines.jl 67.51% <100.00%> (+15.71%) ⬆️
src/plot.jl 61.44% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d18be2e...6c0245c. Read the comment docs.

@simonschoelly
Copy link
Member

Hi,

Sorry for the huge delay here. I like this feature, but I am not sure if chainGraph is the best name - maybe we can come with a more intiutive name?

@@ -35,9 +60,17 @@ function graphline(g::AbstractGraph{T}, locs_x, locs_y, nodesize::Real, arrowlen
endx = locs_x[j] + nodesize*cos(θ+π)
endy = locs_y[j] + nodesize*sin(θ+π)
lines[e_idx] = [(startx, starty), (endx, endy)]
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
if chainGraph
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have used tabs here. For consistency with the rest of the project we should spaces instead.

end

function filterUndef(array)
result = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result = []
result = eltype(array)[]

return has_edge(g,dst(e),src(e))
end

function filterUndef(array)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function filterUndef(array)
function filterAssigned(array)

or filterDefined.

Reason is, that when one uses the filter function then predicate specifies what should stay in the array and not what should be filtered out.

Comment on lines +9 to +11
if result == []
result = typeof(array[i])[]
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if result == []
result = typeof(array[i])[]
end

We can remove this as we can already initialize the list correctly above.

Comment on lines +35 to +43
if chainGraph
if !hasReverseEdge(g,e)
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
end
else
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if chainGraph
if !hasReverseEdge(g,e)
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
end
else
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
end
if !(chainGraph && !hasReverseEdge(g, e))
arr1, arr2 = arrowcoords(θ, endx, endy, arrowlength, angleoffset)
arrows[e_idx] = [arr1, (endx, endy), arr2]
end

@@ -1,7 +1,24 @@
function hasReverseEdge(g, e)
return has_edge(g,dst(e),src(e))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return has_edge(g,dst(e),src(e))
return has_edge(g, dst(e), src(e))

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.

2 participants