(Default)Dict for some attributes

[filchristou]

There was some talk about a newer interface taking advantage of the utils.jl/getattr() method.
#67 (comment)

It happened that I needed it for some arguments and I developed this PR for a possible implementation.

I implemented it just for nlabels_textsize and node_size.
If you agree with the implementation it's rather straight forward to expand it for all other arguments.

DefaultDict

I didn't really like the idea of bringing in a dependency for DataStructures.jl, but as much I thought about it, I couldn't find a way to leverage multiple dispatch to handle it transparently.
As a result I added one more check to the the getattr() method.
I think overall it is a nice feature though, to have an DefaultDict as an input.

Example

julia> using Graphs, DataStructures
julia> gc = circular_ladder_graph(5)
julia> graphplot(gc; nlabels=repr.(vertices(gc)), nlabels_textsize=Dict(9=>0, 10=>0), node_size=Dict(9=>0, 10=>0))
julia> graphplot(gc; nlabels=repr.(vertices(gc)), nlabels_textsize=DefaultDict(30,Dict(9=>0, 10=>0)), node_size=DefaultDict(5,Dict(9=>0, 10=>0)))

Generating the following plots:

Conclusions

This PR is not meant to be merged without any discussion.
Instead, I wanted to suggest a way of implementing the new interface and talk it over.
Personally I believe that if we want to do exactly this for all attributes, the code could get messy.
So I think in the future it would be good to do all these new lifts inside a function or a macro.

[codecov-commenter]

Codecov Report

Patch coverage: 94.59% and project coverage change: +0.34 🎉

Comparison is base (7c2e706) 79.76% compared to head (e0d58c7) 80.11%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   79.76%   80.11%   +0.34%     
==========================================
  Files           5        5              
  Lines         687      704      +17     
==========================================
+ Hits          548      564      +16     
- Misses        139      140       +1     

Impacted Files	Coverage Δ
src/GraphMakie.jl	100.00% <ø> (ø)
src/recipes.jl	93.28% <86.66%> (-0.32%)	⬇️
src/utils.jl	90.82% <100.00%> (+1.57%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hexaeder]

Overall I am in favour of doing this. get_attr was added much later when i needed to handle edge parameters very differently depending on edgeplot using linesegmens or beziersegments. I never took the the time to properly expand the usage to all the arguments. Maybe it would be worth looking how it is done in Makie itself, the problem of distributing attributes which may be given in various formats to their individual recipients must be handled there all the time.

Other things to consider:

• Makie can handle observables of single values. I think it would be bad to @lift nodesize=5 to an vector. This might be a problem bad for very large networks.
• The code should be extended to relift on size changes, but thats easily doable.

Just brainstroming here, but maybe we should do something like

prep_arguments(x, size, default) = x
function prep_arguments(x::Observable{AbstractDict}, size, default)
    @lift [collect based on $size and $x and $default]
end
function prep_arguments(x::Observable{DefaultDict}, size, _)
    @lift [collect based on $size and $x]
end

so that in the recipe the code becomes something like markersize=prep_arguments(gp.nodesize, size, default).
But thats not good if the arguments come in as Observable{Any}...

[hexaeder]

Sorry i totally forgot about that PR. And also I am an Idiot: i didn't like how it adds another dep, however, we already implicitly depend on DataStructures (via Graphs and others) so this really is a non-issue.

I am not a huge fan of the implementation of DefaultDict because it does not behave like I'd expect for get. But it is available and used so better to build on that then to cook up something new. So I'd say if you want to add this for more arguments go for it. The very least would be to document its usage for the parts where it is already implemented in that PR and add some tests.

[filchristou]

you made some valuable comments above and I didn't have time to process them in detail. Especially looking into the Makie package to see how they are doing it. So if you have more concrete examples of how Makie is doing it please let me know.

So I'd say if you want to add this for more arguments go for it.

I would prefer that we first decide on the structure of the code, merge this PR as a prototype, and then we can slowly start working on gradually doing the same for all attributes as a non-brainer.

I am already using this feature locally, so sooner or later I will focus on finalizing & merging this PR..

[filchristou]

for a demo you can try the following:

julia> using GraphMakie, GLMakie, Graphs;

julia> gc = circular_ladder_graph(5);

julia> ons = Observable(30);

julia> onf = Observable(30);

julia> graphplot(gc; nlabels=repr.(vertices(gc)), node_size=ons, nlabels_fontsize=onf)

julia> ons[] = 10; # check changes

julia> onf[] = 10; # check changes

julia> ons = Observable(Dict(1=>30, 10=>30));

julia> onf = Observable(Dict(1=>30, 10=>30));

julia> graphplot(gc; nlabels=repr.(vertices(gc)), node_size=ons, nlabels_fontsize=onf)

julia> onf[] = Dict(7=>30); # check changes

julia> ons[] = Dict(7=>30); # check changes

julia> ons = Observable(DefaultDict(70, 1=>30, 10=>30));

julia> onf = Observable(DefaultDict(70, 1=>30, 10=>30));

julia> graphplot(gc; nlabels=repr.(vertices(gc)), node_size=ons, nlabels_fontsize=onf)

julia> ons[] = DefaultDict(20, 10=>70); # check changes

julia> onf[] = DefaultDict(20, 10=>70); # check changes

[filchristou]

Other things to consider:

• Makie can handle observables of single values. I think it would be bad to @lift nodesize=5 to an vector. This might be a problem bad for very large networks.
• The code should be extended to relift on size changes, but thats easily doable.

[filchristou]

@hexaeder please let me know what do you think. As already mentioned this PR is a prototype and the other arguments should follow rather easily. The documentation and tests you mention should follow after we reach an agreement :)

[hexaeder]

nice, example works great!

[filchristou]

Actually I also thought about a small change that in my opinion will lead to cleaner code (non-disruptive):

Change function getattr(o::Observable, idx, default=nothing) to

getattr(o::Observable, idx, default=nothing) = getattr(o[], idx, default

Add

function getattr(x, idx, default=nothing)
    if x isa AbstractVector && !isa(x, Point)
        return x[idx]
    elseif x isa DefaultDict || x isa DefaultOrderedDict
        return getindex(x, idx)
    elseif x isa AbstractDict
        return get(x, idx, default)
    else
        return x === nothing ? default : x
    end
end

So then the loop can be

@lifft begin
...
[getattr($o, i, $default) for i in 1:$size]
...
end

intead of

@lift begin
...
[getattr(o, i, $default) for i in 1:$size]
...
end

which I always find a bit awkward to handle observables as observables and not values inside a @lift.

What do you think ?

[hexaeder]

Sure, you may redefine getattr that way. While you're at it: you can use the same issingleargument function ther since it means the same thing.

Have you looked into applying those things to the edge attributes yet? There it is a bit more complicated since edgeplot either calls linesegmens (which needs single attribute or vetor so we need to prep_arguments) or bezierpathes (which makes use of getattr anyway and can handle dicts and defaultdicts). So the prep_arguments shoult probably be applied inside the linesegments path, but i havn't really thought this through.

[hexaeder]

Also, since getattr, prep_arguments and issingleattribute are all part of the same processing i think they should live next to each other in the codebase. I don't care whether thats recipe or utils. Also, mache prep_arguments should be called prep_attributes or something?

[filchristou]

I see. No I still haven't looked into edges.

prep_arguments should be called prep_attributes or something?

yes, sound better.

[filchristou]

Since we started this work, I think it's better we might as well finish off all attributes in this PR.
This way also docs/tests can be made more complete.

Following a list of the attributes and which ones can support the new feature:

• layout = Spring(),

node attributes (Scatter)

• node_color = scatter_theme.color,
• node_size = scatter_theme.markersize,
• node_marker = scatter_theme.marker,
• node_attr = (;),

edge attributes (LineSegements)

• edge_color = lineseg_theme.color,
• edge_width = lineseg_theme.linewidth,
• edge_attr = (;),

arrow attributes (Scatter)

• arrow_show = automatic,
• arrow_marker = '➤',
• arrow_size = scatter_theme.markersize,
• arrow_shift = 0.5,
• arrow_attr = (;),

node label attributes (Text)

• nlabels = nothing,
• nlabels_align = (:left, :bottom),
• nlabels_distance = 0.0,
• nlabels_color = labels_theme.color,
• nlabels_offset = nothing,
• nlabels_fontsize = labels_theme.fontsize,
• nlabels_attr = (;),

edge label attributes (Text)

• elabels = nothing,
• elabels_align = (:center, :center),
• elabels_side = :left,
• elabels_distance = automatic,
• elabels_shift = 0.5,
• elabels_rotation = automatic,
• elabels_offset = nothing,
• elabels_color = labels_theme.color,
• elabels_fontsize = labels_theme.fontsize,
• elabels_attr = (;),

self edge attributes (already use getattr so no need ? )

• edge_plottype = automatic,
• selfedge_size = automatic,
• selfedge_direction = automatic,
• selfedge_width = automatic,
• curve_distance = 0.1,
• curve_distance_usage = automatic,
• tangents=nothing,
• tfactor=0.6,
• waypoints=nothing,
• waypoint_radius=nothing,

[filchristou]

I start to be happy with where this goes and I think we are close to merging.
Have a look at my remarks here where I make a summary and I highlight the new additions.
I took some liberty on some things so feel free to be critical about them.

Summary

Mainly, this PR extends functionality by supporting arguments of Dict.
However there are some other ideas integrated.

Bug Fixes

Testing my previous workflow #89 (comment)
there were some errors. More specifically updating the Observable properties errored. I fixed the bug in the following lines

• https://github.com/MakieOrg/GraphMakie.jl/pull/89/files#diff-8861ed4224a2baebdb2c6efa750e27a9d73b2b84f72f4f46b153786969fd7f8eL259
• https://github.com/MakieOrg/GraphMakie.jl/pull/89/files#diff-8861ed4224a2baebdb2c6efa750e27a9d73b2b84f72f4f46b153786969fd7f8eL304
• https://github.com/MakieOrg/GraphMakie.jl/pull/89/files#diff-8861ed4224a2baebdb2c6efa750e27a9d73b2b84f72f4f46b153786969fd7f8eR747
  The problem was that in the @lift command not the Observable type was used.

Renaming variables

In lines like

GraphMakie.jl/src/recipes.jl

Line 206 in 7c2e706

graph = gp[:graph]

,

GraphMakie.jl/src/recipes.jl

Line 235 in 7c2e706

node_pos = gp[:node_pos]

,

GraphMakie.jl/src/recipes.jl

Line 286 in 7c2e706

edge_paths = gp[:edge_paths]

,
the same name as the recipe Observable is given to represent same reference.
This was in contrast to how node_color, node_marker, node_strokewidth are meant which are different dependent Observable. I think it's more clear to give them different name. I opted for name_m where "m" stood for modified.

Extension of Dict to all supporting arguments

The main work of this PR is modifying get_attr and adding the prep_arguments functions as already discussed some months ago. Now, I just inlined the call to this function to all appropriate recipes.

Use of Dict{Edge} for edge properties

We 've talked in the past about supporting Dict{Edge} for edge properties.
At the beginning I was hesitant to implement something like that since for big graphs it could get slow and memory hungry.
However I think with the current design it's okey.
Already at compile time (or one dynamic dispatch per graphplot) the correct flavor of getedgeinds is called that returns the appropriate indexing. Only when the graph is undirected and an AbstractDict{<:AbstractEdge} is given there is some further computation, which is being done on demand using Iterators rather than saving it in memory.
To further increase efficiency we can use some lazy implementation that caches the result.

SimpleTraits.jl dependency

SimpleTraits.jl is already in the Manifest.toml as it's a dependency of Graphs.jl.
I find it elegant to use it also here.

Docs

I updated the docs to talk about the new feature.
I don't have examples there but I reference the reftests

Reftests

I introduced quite some reftests.
I wanted to introduce them somewhere in the middle of the document as they would be better categorized, but I realized I had to change all the assets numbering. Is there a clever way to do that, or should we always add reftests in the end to avoid such developer overheads ?

Versioning

This is non-breaking but still introduces some obvious new features. Should that be a minor or patch update ?

docs/Manifest.toml

The ./docs should depend on the GraphMakie as dev ... Otherwise it wouldn't follow locally up until the version was published.
Am I right ? How do you develop locally when there are such changes ?
If you like, this could be a separate PR.

Remarks

I just wanted to take the opportunity to highlight some points

element-wise automatic

Currently the Dict funcionality can have a single default value.
However, some arguments like label rotation etc. have different default values per element.
I think we should support that the fallback for Dict and DefaultDict should be these default values.
However, I would advise that to be a separate PR.

Need for benchmarks

As I mentioned I was a bit concerned with performance and currently there is no out-of-the-box way to test how we do.
There are also some other julia packages we can compare to, or even to the python ecosystem.
This can bring in a new dimension to this package, which I think is desirable.

[hexaeder]

Funcionally I am on board but I am not super happy with this construct. Because a change of gp.edge_color from the outside would trigger two things:

• recalculation of observable definition within prep_attributes
• recalculation of getedgeinsd output which then triggers recalculation of prep_attributes

Havn't though this through, but maybe it is worth splitting prep_attributes in

prep_node_attributes(attr::Observable, graph::Observable{g}, default)

and

prep_edge_attributes(attr::Observable, graph::Observable{g}, default)
    @lift begin
        if issingleattribute($attr)
            isnothing($attr) ? $default : $attr
        elseif $attr isa AbstractVector
            $attr
        else
            [getattr($attr, i, $default) for i in getedgeinds($graph, $attr)]
        end
    end
end

So @lift getedgeinds(..) for edges and @lift vertice(g..) could become part of the prep_attributes lift.

[filchristou]

If gp.edge_color changes then tempobs = @lift(getedgeinds($graph, $(gp.edge_color)) is updated and then the @lift inside inside prep_arguments. To me this sounds sequential and all right.

recalculation of getedgeinsd output which then triggers recalculation of prep_attributes

Isn't this the same as the first point of recalculating the tempobs I mentioned above ?

Could you explain your concerns once more ?

I indeed thought about splitting it but I didn't want to be intrusive in our previous design, so I ended up putting another layer of the getedgeinds thing on top. The problem was that prep_attributes would need information of the graph like IsDirected, so we will need to pass in further arguments. In the end I preferred to separate the two functionalities:

• get keys (trivial for vertices)
• call prep_arguments with the indices/keys generated

It also looked more modular.

[hexaeder]

There are two subproblems:

• even if gp.edge_color is an vector, the tempobs will be updated every single time
• the observable created in prep_attributes lifts both on gp.edge_color and on the tempobs. Therefore, change of gp.edge_color will trigger prep_attributes, then tempobs and then tempobs will trigger prep_attributes again.

I think this function

prep_edge_attributes(attr::Observable, graph::Observable{g}, default)
    @lift begin
        if issingleattribute($attr)
            isnothing($attr) ? $default : $attr
        elseif $attr isa AbstractVector
            $attr
        else
            [getattr($attr, i, $default) for i in getedgeinds($graph, $attr)]
        end
    end
end

elides both of those problems. I think withint prep_edge_attributes it still makes perfect sense to use the trait dispatches function getedgeinds. But we get rid of one layer of observables.

[filchristou]

You are right! Nice catch! I though there was a dependency tree that decided what/how to execute with Observables.
However the following code which is similar indeed is executed twice:

using Observables

edge_color = Observable(1)
graph = Observable(2)

tempobs = map(edge_color, graph) do e,g
    @info("tempobs update")
    e + g
end

prep_attrs = map(edge_color, tempobs) do e, t
    @info("prep_attrs update")
    e + t
end

myplot = map(prep_attrs) do p
    @info("myplot update")
    p * 10
end

Changing edge_color[] will execute prepr_attrs twice:

julia> edge_color[] = 3;
[ Info: tempobs update
[ Info: prep_attrs update
[ Info: myplot update
[ Info: prep_attrs update
[ Info: myplot update

I will rewrite it as you suggested together with the other comments.
Also in order to keep it symmetric I would also make a prep_vertex_attributes that also gets a graph Observable.

[hexaeder]

I think it's more clear to give them different name. I opted for name_m where "m" stood for modified.

totally agree

Use of Dict{Edge} for edge properties

see comments on files, i think we can reduce some unecssary updates by splitting prep arguments in two functions for nodes and edges

I wanted to introduce them somewhere in the middle of the document as they would be better categorized, but I realized I had to change all the assets numbering. Is there a clever way to do that, or should we always add reftests in the end to avoid such developer overheads?

No, not yet. There is a bit of a tradeoff, currently we only need to annotate with @reftest and number them automaticially by file name. I have an not-yet-registered package PlotReferenceTests.jl which does things similar but slightly different, there you'd need to give every reftest a name @reftest fig "example". I think it makes sense to switch eventually as it also works much better when interactivly stepping through those tests out of order (as I often do during debugging).

Versioning

I see that it is a big feature upgrade but I'd still opt for non-breaking release. I think there is no reason to make everybody bump there versions.

docs/Manifest.toml

What i normally do is executing docs/localmake.jl from a terminal. It will set up the doc env similar to what happens in CI. And if you have LiveServer in your global env it will serve the docs and you can repeatadly rebuild them without restarting the julia session. Even though I just noticed that there should be a using Revise in there too.

element-wise automatic

I agree, this changes the structure quit a bit but is relatively simple and should be done in a separate PR. Basicially we would first need to expand/prep the arguments before resolving the automatics, which is currently done first creating those _m version.

benchmarks

Agree. I think there is room for a lot of improvement in the package, currently everything is super dynamic and type unstable.

[filchristou]

Regarding Manifest.toml I will delete it then. I just can't wait for JuliaLang/Pkg.jl#492

[hexaeder]

Regarding Manifest.toml I will delete it then. I just can't wait for JuliaLang/Pkg.jl#492

Sorry maybe I missunderstood you: so you were suggesting to add the Manifest in the repo? Normally I just keep my docs/Manifest.toml locally with dev ... I think there are good arguments for both....

[filchristou]

Regarding the element-wise automatic thing, theoretically that would be breaking if we introduce it in a version later than the one of this PR.

[hexaeder]

I'll merge this into a feature branch of the repo to get the doc preview build, you should have all the rights to push to that new branch too