Implement graph, nodes and edge creation #3

codejaeger · 2021-06-14T17:08:26Z

GraphAnimation

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

Did I update CHANGELOG.md with whatever changes/features I added with this PR?
Did I make sure to only change the part of the file where I introduced a new change/feature?
Did I cover all corner cases to be close to 100% test coverage (if applicable)?
Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
Did I properly add test dependencies to the test directory (if applicable)?
Did I check relevant tutorials that may be affected by changes in this PR?
Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Closes #

How did you address these issues with this PR? What methods did you use?

Wikunia

Just some comments ;)

src/structs/Graphs.jl

* GraphAnimation * GraphNode * GraphEdge

@object

* Add macro syntax @object as temporary sol. for predefined objects * Update graph creation workflow * Add utility graph type WeightedGraph

src/structs/JGraph.jl

Wikunia

Looks good. Just some questions before I go deeper 😉
Maybe you can add some flow diagram/description of how everything is connected and give some details/docstrings to some more functions. Mostly describing what they are used for/doing.

Wikunia · 2021-06-26T13:56:47Z

src/structs/GraphEdge.jl

+        @warn "Edge $(from_node)=>$(to_node) is already created on canvas. Recreating it will leave orphan edges in the animation. To undo, call `rem_edge!`"
+    end
+    add_edge!(g.adjacency_list.graph, from_node, to_node)
+    set_prop!(g.adjacency_list, from_node, to_node, length(g.ordering)+1)


what is set here? What does the prop represent, is it the id of the edge?

set_prop! sets the property of the node/edge (edge in this case). Since the WeightedGraph type can only have one property for each node and edge there is no name given to that property.

Wikunia · 2021-06-26T13:57:50Z

src/structs/GraphVertex.jl

+    property_style_map::Dict{Any,Symbol} = Dict{Any,Symbol}(),
+)
+    g = graph.meta
+    if !(typeof(g) <: JGraph)


Maybe we can do a parameterized AbstractObject instead of checking the type of the meta field here.

Wikunia · 2021-06-26T13:59:52Z

src/structs/GraphVertex.jl

+            draw_fn = draw
+        end
+        add_vertex!(g.adjacency_list.graph)
+        set_prop!(g.adjacency_list, nv(g.adjacency_list), length(g.ordering)+1)


I'm a bit confused by the set_prop functionality in general I think. I thought we can set different properties for a node, edge but it seems like we can only store one thing? Which in this case is the length(g.ordering)+1 ?

Yes, to store more than one property for nodes and edges there is already the MetaGraph package. In many graph algorithms, only a single weight is associated with the nodes and edges hence this would be a useful structure to have.

Wikunia · 2021-06-26T14:01:29Z

src/structs/JGraph.jl

+    end
+    # Now assign positions back to all nodes
+    for (idx, p) in enumerate(node_props(g.adjacency_list))
+        g.ordering[p].meta.opts[:position] = object.opts[:layout][idx]


is this used to give the position to each node later in rendering?

Wikunia

Additionally to the specific comments in the code:
You often restrict the types of arguments in your functions where it isn't necessary and sometimes is restricting the user.
Furthermore you can restructure parts of your functions to call helper functions to limit code copy pasting.

src/Javis.jl

Wikunia · 2021-07-05T08:16:19Z

examples/graph_animations/demo.md

+count = 0
+for i in 1:length(adjacency_list)
+    for j in adjacency_list[i]
+        @Graph g 15+count*10:150 GraphEdge(i, j, (args...; kwargs...)->edge(; kwargs...)) O


doesn't work with count as it's not known when you write this in global scope

Wikunia · 2021-07-05T08:18:04Z

src/node_shapes.jl

+    opts = Dict{Symbol, Any}()
+    opts[:shape] = shape
+    opts[:clip] = clip
+    if shape == :rectangle


currently there is no error message when one writes:

node_shape(:rectangle, true, radius=12)

Maybe have a look at: https://docs.julialang.org/en/v1/manual/types/#%22Value-types%22 to dispatch on the Symbol

src/node_shapes.jl

Wikunia · 2021-07-05T08:24:27Z

src/node_shapes.jl

+
+# Arguments
+- `fill::Symbol`: Can be `image` or `color`.
+- `arg::String`: Can be a color like "red" or a image path in the same directory like "image.png".


In Luxor one normally sets colors with symbols so :red instead of "red"

Hey, I was using sethue to switch between colors. I checked the methods available for sethue and could not find one using Symbols. Is there something else you are referring to here?

Oh sorry I think I got confused 😁 but I think other color packages are also supported. Let me check though

See: https://juliagraphics.github.io/Luxor.jl/stable/colors-styles/

Wikunia · 2021-07-05T08:25:12Z

src/node_shapes.jl

+- `color`: The color of the text. Default is "black".
+- `offset`: Used only when `align` is not `:inside`. Specifies an offset in the direction of `align`.
+"""
+function node_text(text::AbstractString, align::Symbol, color::String="black", offset::Real=1)


I think we want to have an option to specify the font size?

To specify font and size, it is possible to add a (args...; kwargs...) -> setfont(font, size) anywhere in the list before a node_text is created. Since these functions are supposed to be backup options if the user wants to skip some drawing properties I skipped that option. Do you think it is still needed?

I think font size is something so standard that we want to include it in node_text

Okay sure will do that.

src/node_shapes.jl

Wikunia

Just some minor things

Wikunia · 2021-07-06T11:31:55Z

src/edge_shapes.jl

+            return
+        end
+        if self_loop
+            # Need to find out where to draw the edge without clutter


Set a Todo: in those cases to find them later

Wikunia · 2021-07-06T11:35:40Z

src/edge_shapes.jl

@@ -0,0 +1,44 @@
+function edge_shape(shape::Symbol=:line, clip::Bool = false; center_offset::Real = 3, end_offsets::Tuple{Real, Real} = (0, 0), width::Real = 2, dash::String = "solid")


please add a docstring here

src/edge_shapes.jl

src/Javis.jl

Wikunia · 2021-07-07T11:48:55Z

src/structs/JGraph.jl

+    # Check available layouts
+    if !(layout in [:none, :spring, :spectral])
+        layout = :spring
+        @warn "Unknown layout '$(layout)'. Defaulting to static layout"


do you mean: Defaulting to :spring layout?

Wikunia · 2021-07-07T11:58:34Z

examples/graph_animations/demo.md

+video = Video(400, 400)
+Background(1:150, ground)
+
+g = @Object 1:150 JGraph(true, 100, 100) O


Seems like the O doesn't have any effect?
For example replacing it by Point(-100,0) does not shift it 100px to the left

Wikunia · 2021-07-07T12:02:53Z

src/structs/JGraph.jl

+
+Create an empty graph on the canvas.
+"""
+JGraph(directed::Bool, width::Int, height::Int) =


this should allow the keywords mode, layout, get_node_attribute, get_edge_attribute, right?

Wikunia · 2021-07-07T15:42:58Z

src/structs/JGraph.jl

@@ -53,6 +53,10 @@ JGraph(directed::Bool, width::Int, height::Int) =
    directed ? JGraph(WeightedGraph(LightGraphs.SimpleDiGraph()), width, height) :
    JGraph(WeightedGraph(LightGraphs.SimpleGraph()), width, height)

+JGraph(directed::Bool, width::Int, height::Int, layout::Symbol=:none) =


mode is still missing, right?

Wikunia · 2021-07-07T15:51:32Z

examples/graph_animations/demo2.md

+    return Dict(:position=>position), (args...; kwargs...) -> nothing
+end


this notation seems a bit weird. What are the two return arguments?

* TODO: Label position for bezier curves

codejaeger force-pushed the signatures_and_docstrings branch from f01d09b to b13adfa Compare June 14, 2021 18:41

Wikunia reviewed Jun 14, 2021

View reviewed changes

src/structs/Graphs.jl Outdated Show resolved Hide resolved

src/structs/Graphs.jl Outdated Show resolved Hide resolved

src/structs/Graphs.jl Outdated Show resolved Hide resolved

codejaeger force-pushed the signatures_and_docstrings branch from b13adfa to 1b2e7fa Compare June 15, 2021 11:26

Update with docstrings

8709707

* GraphAnimation * GraphNode * GraphEdge

codejaeger force-pushed the signatures_and_docstrings branch from 1b2e7fa to 8709707 Compare June 15, 2021 21:05

codejaeger added 3 commits June 17, 2021 01:53

Fill in implementation details for GraphAnimation

545469b

Refactor syntax for using graph animations

ee3bd3d

Update

b325b0e

* Add macro syntax @object as temporary sol. for predefined objects * Update graph creation workflow * Add utility graph type WeightedGraph

codejaeger changed the title ~~Update with docstrings~~ Implement graph, nodes and edge creation Jun 21, 2021

Wikunia reviewed Jun 22, 2021

View reviewed changes

src/structs/JGraph.jl Outdated Show resolved Hide resolved

Update with GraphVertex creation

b0c7f93

codejaeger force-pushed the signatures_and_docstrings branch from a3f0f79 to b0c7f93 Compare June 22, 2021 20:17

codejaeger added 2 commits June 25, 2021 15:16

Fix macro @graph

692508a

Update with demo

907daa8

codejaeger force-pushed the signatures_and_docstrings branch from bd37517 to 907daa8 Compare June 26, 2021 12:49

Wikunia reviewed Jun 26, 2021

View reviewed changes

Wikunia suggested changes Jul 5, 2021

View reviewed changes

codejaeger force-pushed the signatures_and_docstrings branch from 2abb72d to b3c4846 Compare July 5, 2021 14:06

Add node styling options

be61303

codejaeger force-pushed the signatures_and_docstrings branch from b3c4846 to be61303 Compare July 5, 2021 14:11

Add edge drawing functions

8e584a3

Wikunia reviewed Jul 6, 2021

View reviewed changes

Wikunia reviewed Jul 7, 2021

View reviewed changes

src/Javis.jl Show resolved Hide resolved

Wikunia reviewed Jul 7, 2021

View reviewed changes

codejaeger force-pushed the signatures_and_docstrings branch from e960aa5 to 5129cf8 Compare July 7, 2021 15:45

Wikunia reviewed Jul 7, 2021

View reviewed changes

codejaeger force-pushed the signatures_and_docstrings branch from de1ba34 to 84062b4 Compare July 7, 2021 16:12

Update with custom node positions

58e60b4

codejaeger force-pushed the signatures_and_docstrings branch from 84062b4 to 58e60b4 Compare July 10, 2021 15:40

codejaeger added 2 commits July 13, 2021 16:51

Add self loops

f96c53f

Add edge style and arrows

887fb1e

codejaeger marked this pull request as draft July 16, 2021 16:24

Add some basic edge labelling

b64d014

* TODO: Label position for bezier curves

codejaeger force-pushed the signatures_and_docstrings branch from 3c8cfec to b64d014 Compare July 16, 2021 17:29

		@@ -0,0 +1,44 @@
		function edge_shape(shape::Symbol=:line, clip::Bool = false; center_offset::Real = 3, end_offsets::Tuple{Real, Real} = (0, 0), width::Real = 2, dash::String = "solid")

		return Dict(:position=>position), (args...; kwargs...) -> nothing
		end

Implement graph, nodes and edge creation #3

Are you sure you want to change the base?

Implement graph, nodes and edge creation #3

Uh oh!

Conversation

codejaeger commented Jun 14, 2021

PR Checklist

Uh oh!

Wikunia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wikunia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wikunia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codejaeger Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wikunia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codejaeger Jul 5, 2021 •

edited

Loading