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

[WIP] Non-Working Example for custom shapes #138

Merged
merged 8 commits into from
Nov 18, 2020
Merged

[WIP] Non-Working Example for custom shapes #138

merged 8 commits into from
Nov 18, 2020

Conversation

enuit
Copy link
Contributor

@enuit enuit commented Nov 8, 2020

Following the discussion in #136 I set up a simple extension to allow for custom shapes. I follow the proposal from @JackDevine to use a function taking position and nodewidth/height as input parameters. An example function would look like this:

function customshape(x_i,y_i,nw, nh)
	out = Tuple{Float64, Float64}[(0,0),(0,1),(1,1),(1,0),(0,0)]
	map(out) do t
		(t[1]* nw + x_i -nw/2, t[2]*nh + y_i - nh/2)
	end
end
graphplot(rand(4,4), nodeshape=Plots.Shape([1,2,2], [1,2,1]))

However, this approach doesn't work. There is an error

graphplot(rand(3,3), nodeshape=customshape)
┌ Warning: markershape customshape is unsupported with Plots.GRBackend().  Choose from: [:none, :auto, :circle, :rect, :star5, :diamond, :hexagon, :cross, :xcross, :utriangle, :dtriangle, :rtriangle, :ltriangle, :pentagon, :heptagon, :octagon, :star4, :star6, :star7, :star8, :vline, :hline, :+, :x]
└ @ Plots ~/.julia/packages/Plots/D7Ica/src/args.jl:1198
┌ Warning: markershape customshape is unsupported with Plots.GRBackend().  Choose from: [:none, :auto, :circle, :rect, :star5, :diamond, :hexagon, :cross, :xcross, :utriangle, :dtriangle, :rtriangle, :ltriangle, :pentagon, :heptagon, :octagon, :star4, :star6, :star7, :star8, :vline, :hline, :+, :x]
└ @ Plots ~/.julia/packages/Plots/D7Ica/src/args.jl:1198
Error showing value of type Plots.Plot{Plots.GRBackend}:
ERROR: MethodError: no method matching gr_draw_marker(::Plots.Series, ::Float64, ::Float64, ::Tuple{Float64,Float64}, ::Int64, ::Float64, ::Int64, ::typeof(customshape))
Closest candidates are:
  gr_draw_marker(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Plots.Shape) at packages/Plots/D7Ica/src/backends/gr.jl:295
  gr_draw_marker(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Symbol) at packages/Plots/D7Ica/src/backends/gr.jl:325
Stacktrace:
 [1] gr_draw_markers(::Plots.Series, ::Array{Float64,1}, ::Array{Float64,1}, ::Tuple{Float64,Float64}, ::Float64, ::Int64) at packages/Plots/D7Ica/src/backends/gr.jl:1646
 [2] gr_draw_markers(::Plots.Series, ::Array{Float64,1}, ::Array{Float64,1}, ::Tuple{Float64,Float64}) at /packages/Plots/D7Ica/src/backends/gr.jl:1634
 [3] gr_add_series(::Plots.Subplot{Plots.GRBackend}, ::Plots.Series) at packages/Plots/D7Ica/src/backends/gr.jl:1519
 [4] gr_display(::Plots.Subplot{Plots.GRBackend}, ::Measures.Length{:mm,Float64}, ::Measures.Length{:mm,Float64}, ::Array{Float64,1}) at packages/Plots/D7Ica/src/backends/gr.jl:891
 [5] gr_display(::Plots.Plot{Plots.GRBackend}, ::String) at packages/Plots/D7Ica/src/backends/gr.jl:596
 [6] gr_display at packages/Plots/D7Ica/src/backends/gr.jl:560 [inlined]
 [7] _display(::Plots.Plot{Plots.GRBackend}) at julia/packages/Plots/D7Ica/src/backends/gr.jl:1805
 [8] display(::Plots.PlotsDisplay, ::Plots.Plot{Plots.GRBackend}) at packages/Plots/D7Ica/src/output.jl:150
 [9] display(::Any) at ./multimedia.jl:328
 [10] #invokelatest#1 at ./essentials.jl:710 [inlined]
 [11] invokelatest at ./essentials.jl:709 [inlined]
 [12] print_response(::IO, ::Any, ::Bool, ::Bool, ::Any) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:238
 [13] print_response(::REPL.AbstractREPL, ::Any, ::Bool, ::Bool) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:223
 [14] (::REPL.var"#do_respond#54"{Bool,Bool,REPL.var"#64#73"{REPL.LineEditREPL,REPL.REPLHistoryProvider},REPL.LineEditREPL,REPL.LineEdit.Prompt})(::Any, ::Any, ::Any) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:822
 [15] #invokelatest#1 at ./essentials.jl:710 [inlined]
 [16] invokelatest at ./essentials.jl:709 [inlined]
 [17] run_interface(::REPL.Terminals.TextTerminal, ::REPL.LineEdit.ModalInterface, ::REPL.LineEdit.MIState) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/REPL/src/LineEdit.jl:2355
 [18] run_frontend(::REPL.LineEditREPL, ::REPL.REPLBackendRef) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/REPL/src/REPL.jl:1144
 [19] (::REPL.var"#38#42"{REPL.LineEditREPL,REPL.REPLBackendRef})() at ./task.jl:356

(Which appears only when I try to use the new customshape function)
However, I don't find the position in the source code where the shapes are drawn. (Or given to Plots.jl to be drawn)
As far as I can deduce it from the error, nodeshapes are given to markershape. Does this happen implicitly by the Recipe Macros?

@JackDevine
Copy link
Member

This is fantastic! Sorry that I didn't get to this earlier, I have been quite busy lately.

I think that I understand the problem now.

Basically, the graphplot recipe will create a :markershape to pass to the plotting backend which gets stored in the plotattributes dictionary. Ultimately, the :markershape field of the dictionary wont get used, since around line 880 we have a series of shapes that get drawn, where the outlines of the shapes are given by nodeperimeters at the bottom of the series.

A way to fix the problem that you are seeing is to put the code:

plotattributes[:markershape] = :none

somewhere in the graphplot recipe. Maybe somewhere high up, where we are defining the default behaviours.

Anyway, I am quite excited to get this feature into the repo! Let me know if you have any questions and I will try to respond in a reasonable amount of time.

@enuit
Copy link
Contributor Author

enuit commented Nov 10, 2020

Thank you, that solved the problem. How would you propose do document the custom nodeshapes?
I also added the option for a shape function taking only three parameters. (Nodewidth is dropped)
The idea is, that this is taken for forms that should scale the same in both dimension.

Additionally, I have added a test Function. Its a simple one, doesn't do much. Do you have any further suggestions on improving the work?

@JackDevine
Copy link
Member

How would you propose do document the custom nodeshapes?

You would need to make a PR to to PlotDocs repo. The relevant function is generate_graph_attr_markdown:
https://github.com/JuliaPlots/PlotDocs.jl/blob/ebd45d6f46e0ec69f61eddf3a2c1f43904ebcf81/src/PlotDocs.jl#L256

The function generates the table that you see here:
http://docs.juliaplots.org/latest/generated/graph_attributes/
I would suggest that you add a little to the description of the nodeshape about your custom shape feature.

I also added the option for a shape function taking only three parameters. (Nodewidth is dropped)

I like it!

With the test function that you have created, you also need to add a test to the file test/figures.jl that runs the function you created and compares the output to the file custom_nodeshape.png that you created.

Also, it seems that I slightly mislead you with the plotattributes dictionary. Luckily, the tests caught my mistake. I got the tests to pass locally by using plotattributes[:markershape] = :circle rather than plotattributes[:markershape] = :none. If you go to the julia REPL and type

]test GraphRecipes

then the package manager should run the tests for your local development branch of GraphRecipes.jl.

Do you have any further suggestions on improving the work?

I have some minor comments on the code itself, but at a high level, I think that this is fantastic.

@enuit
Copy link
Contributor Author

enuit commented Nov 11, 2020

I have added the test and everything appears to be running, thank you for your help 👍

Could you comment on your suggestions to the code itself? I would be happy to learn and make the changes, squish the commits and change it into a final pull request? :)

@JackDevine JackDevine marked this pull request as ready for review November 12, 2020 05:29
@JackDevine
Copy link
Member

Could you comment on your suggestions to the code itself?

I have made some minor comments on the relevant lines. Overall, the code is really good and it looks like you have a pretty solid understanding of Julia/package development.

... squish the commits and change it into a final pull request?

Don't worry about squishing/rebasing/anything else like that. There is a button that I can press that automatically squishes commits at the same time as merging.

@enuit
Copy link
Contributor Author

enuit commented Nov 12, 2020

I have made some minor comments on the relevant lines. Overall, the code is really good and it looks like you have a pretty solid understanding of Julia/package development.

I... can't find them. They should show up either when I click on 'Files Changed' or 'Commits' in this pull request, right?

Don't worry about squishing/rebasing/anything else like that. There is a button that I can press that automatically squishes commits at the same time as merging.

Okay, good to know.

src/graphs.jl Outdated
@@ -487,6 +487,9 @@ more details.
nodeheight = (yextent[2] - yextent[1])
push!(node_vec_vec_xy, partialellipse(0, 2π, [x[i], y[i]],
80, nodewidth/2, nodeheight/2))
elseif applicable(nodeshape[node_number], x[i], y[i], 0., 0.)
nodeheight = (yextent[2] - yextent[1])
push!(node_vec_vec_xy, nodeshape[node_number](x[i], y[i], nodewidth/2, nodeheight/2))
else
error("Unknown nodeshape: $(nodeshape[node_number]). Choose from :circle, ellipse, :hexagon, :rect or :rectangle.")
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add something to this error message. Something like: ... Choose from :circle, ellipse, :hexagon, :rect, :rectangle or a custom shape. Custom shapes can be passed as a function customshape such that customshape(x, y, nodeheight, nodewidth) -> nodeperimeter. nodeperimeter must be an array of 2-tuples, where each tuple is a corner of your custom shape, centered at (x, y) and with height nodeheight, width nodewidth.

Additionally, it would be nice to add this feature to the documentation, although that is another PR.

src/graphs.jl Outdated
@@ -355,6 +355,11 @@ more details.
plotattributes[markerproperty] = nodeproperty
end

# Remove the markershapes from the dictionary to allow for nodeshapes not
# compatible with the shapes of plotting ( Custom nodeshape functions don't
# work without this line)
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this comment.

The nodeshape is placed into plotattributes[:markershape] at line 355:

plotattributes[markerproperty] = nodeproperty

However, if plotattributes[:markershape] is a function (or a symbol that the backend doesn't recognize), then the backend will fail to create the marker series even though we ultimately create our own marker series later on in the recipe.

Ultimately, the whole processing of the attributes that I did from lines 337-356 was a bit of a cludge. I think that the code there is ultimately what has generated the whole confusion.

I would recommend that you add the following to line 357;

    # If we pass a value of plotattributes[:markershape] that the backend does not
    # recognize, then the backend will throw an error. The error is thrown despite the
    # fact that we override the default behavior.
    if nodeshape isa Function
        plotattributes[:markershape] = :circle
    end

The conditional is not strictly necessary, but I think it helps to show that we are only changing the disctionary when plotattributes[:markershape] is something that the backend does not recognize.

Feel free to alter the comment a little to make it easier to understand/add your own flare.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't agree with this comment. In particular, I don't understand the phrase "nodeshapes not compatible with the shapes of plotting". The phrase "shapes of plotting" does have a very sentimental/artistic flare, however, is not really specific enough for this context. Rather than "shapes of plotting", I think that you should say "symbols that the backend will recognize as a valid value of plotattributes[:markershape]".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main intention with the comment was to keep the knowledge why this special case had to be introduced. Your code and comment capture that much better. I just added a line to make clarify that the lines where introduced to allow custom nodeshapes.

I changed the code to match yours, the tests then suggested some further changes.

function custom_nodeshapes()
function shapy(x_i,y_i, s)
out = Tuple{Float64, Float64}[(-0.5,0),(0,-0.5),(0.5,0),(0,0.5)]
map(out) do t
Copy link
Member

Choose a reason for hiding this comment

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

There should be four spaces of indentation.

Since people might look at these tests for documentation, it might be nice to add a few different styles so that people can choose a style that they like. The style that I would use (and that I think that you should add to one of the functions) would be:

[(x_i + 0.5s*dx, y_i + 0.5s*dy) for (dx, dy) in [(1,1),(-1,1),(-1,-1),(1,-1)]]

It really is a matter of preference, although it is always nice to cater to all types. You also might want to check that my code returns the right shape. I am more just trying to give you the rough idea here, rather than the exact code

end
function shapy_wh(x_i,y_i, h, w)
out = Tuple{Float64, Float64}[(-0.5,0),(0,-0.5),(0.5,0),(0,0.5)]
map(out) do t
Copy link
Member

Choose a reason for hiding this comment

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

Same formatting issue

@@ -141,6 +141,38 @@ function julia_dict_tree()
plot(TreePlot(d), method=:tree, fontsize=10, nodeshape=:ellipse, size=(1000, 1000))
end

function custom_nodeshapes()
function shapy(x_i,y_i, s)
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent whitespace conventions. I also like to have no spaces after a comma, however, the convention for the JuliaPlots orginisation is to have whitespace after commas, so shapy(x_i, y_i, s). Same for shap_wh below

@JackDevine
Copy link
Member

I... can't find them.

I forgot to click the 'submit review' button! I swear that is a new feature, because I have never seen it before. Sorry about that!

@JackDevine
Copy link
Member

Thanks for going through all of the suggestions 😄

Also, thanks for making the condition for changing plotattributes[:markershape] robust to arrays of nodeshapes and including a test to show that the code can handle arrays of nodeshapes.

I notice that you have a file assets/custom_nodeshapes.png, but I can't see a corresponding test. I suggest that you either delete this file, or make a test for it.

@enuit
Copy link
Contributor Author

enuit commented Nov 17, 2020

Ah, it was a file from the old test. I removed it.

@JackDevine JackDevine merged commit 8135f17 into JuliaPlots:master Nov 18, 2020
@JackDevine
Copy link
Member

The test failure on julia nightly looks unrelated.

Thanks so much!

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