Conversation
Introduced TreeScatter recipe for scatter plots of tree nodes and exported new functions: - Added TreeScatter recipe with customizable attributes - Implemented Makie.plot! function for TreeScatter - Exported treescatter and treescatter! functions - Updated documentation with new treescatter function details - Removed marker-related attributes from TreePlot recipe
- Add nodeordering attribute to TreeScatter recipe - Import AbstractTrees and PreOrderDFS in BasicTreePlotsMakieExt.jl - Update plot! function to use nodeordering for scatter plotting - Add nodeordering documentation in BasicTreePlots.jl
…nd `treecladelabel`
Conflicts: Project.toml took branch compat section
|
This is great! I was thinking if maybe abstracting away some aspects would benefit the code. For example, one could dispatch some of this plotting functions with the treeplot struct or with the expected data type (e.g. Vector of Strings). I like much more: tree = <tree generation function>
tp = treeplot!(tree)
treelabels!(tp; depth = d -> d + 0.1)than the current state: tree = <tree generation function>
tp = treeplot!(tree)
treelabels!(tp.nodepoints; depth = tp.maxtreedepth[] + 0.1)I think is cleaner and more "julian". I would leave treescatter, since we can do a dispatch specifically for the treeplot structure. Synonyms are not terrible when implementing a DSL (which this package basically is), since one can make the code much more readable when using the domain's language. LMKWYT |
|
Agreed, I would like that style of syntax. Annoyingly because these are recipes, the dispatches are not simple. Just passing the plot object to the next recipe leads to some argument errors before even getting to the function body. So It needs some thinking about how to actually pass a plot object to a new recipe. tree = ((:a, :b), (:c, (:d, :e)))
fig, ax, tp = treeplot(tree)
treelabels!(tp)
fig ArgumentError: Failed to construct plot: No plot arguments given.
Stacktrace:
[1] (Plot{BasicTreePlots.treelabels})(user_args::Tuple{}, user_attributes::Dict{Symbol, Any})
@ Makie ~/.julia/packages/Makie/TOy8O/src/compute-plots.jl:717
[2] _create_plot!(::Function, ::Dict{Symbol, Any}, ::Plot{BasicTreePlots.treeplot, Tuple{Tuple{Tuple{Symbol, Symbol}, Tuple{Symbol, Tuple{Symbol, Symbol}}}}})
@ Makie ~/.julia/packages/Makie/TOy8O/src/figureplotting.jl:416
[3] treelabels!(args::Plot{BasicTreePlots.treeplot, Tuple{Tuple{Tuple{Symbol, Symbol}, Tuple{Symbol, Tuple{Symbol, Symbol}}}}}; kw::@Kwargs{})
@ BasicTreePlotsMakieExt ~/.julia/packages/Makie/TOy8O/src/recipes.jl:534
[4] treelabels!(args::Plot{BasicTreePlots.treeplot, Tuple{Tuple{Tuple{Symbol, Symbol}, Tuple{Symbol, Tuple{Symbol, Symbol}}}}})
@ BasicTreePlotsMakieExt ~/.julia/packages/Makie/TOy8O/src/recipes.jl:532
[5] top-level scope
@ ~/projects/BasicTreePlots.jl/test/jl_notebook_cell_df34fa98e69747e1a8f8a730347b8e2f_X26sZmlsZQ==.jl:7I'll take another look. I went to this syntax because it was working and preventing recomputation But, if you figure out a way to directly pass the plot object with the v0.24 syntax, please let me know. I don't think that syntax should hold up these recipes as a whole though. |
Hey @cvgilv
I've finally had some time to work on this repo. Sorry for the delay.
I took your PR #15 and updated the recipies to use Makie v0.24 and fixed the issues with plotting on a polar axis. I did heavily modify the api. So rather than directly providing the tree to
treelabels,treecladelabel, ortreearea. The user providetp.nodepointsthe dictionary ofnode => Point2fpairs as calculated bytreeplot. This reduces the need to recalculate the coordinates and stops the user needing to remember to match the arguments between plotting functions. It also means A lot of the redundant options were removed.Example usage is below
Also note that
tp.orderedpointsis also accessible from thetreeplotplot object. This allows for directly using Makie'sscatterrecipe with all options as thetp.orderedpointsare the nodes of the tree inPreOrderDFSorder. As such I am removing thetreescatterrecipe for now. Though, I could be convinced to bring it back in a separate pull request if there is a use case that the standardscatterrecipe doesn't cover for trees.I'll likely merge this pull request into the main branch in the next week or so. But I would like to improve the documentation a bit before bumping the version number and fully releasing these changes.
If you have some time, do please checkout the branch and let me know of any issues you find. And thanks again for your help with initiating these extra recipes.