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

Add AbstractTrees plotting #80

Merged
merged 3 commits into from
Aug 15, 2019
Merged

Conversation

ericphanson
Copy link
Contributor

(The first commit is #79)

AbstractTrees.jl is a package that allows one to define methods for AbstractTrees.children, and then traverse trees with various algorithms. AbstractTrees includes a print_tree function which then pretty prints the tree, converting nodes to text via printnode. This pull request adds a recipe to do the same graphically, using children to construct the tree and printnode again for the labels.

Since "AbstractTrees" can be of any type, I also added a TreePlot wrapper to dispatch the recipes on. I think that's the right way to go because abstract trees are quite general so there may be more that one way to plot them. For example, since AbstractTrees provides a fallback for children on iterables, on this branch plot(TreePlot([ [1:10], [1:5] ])) generates a tree plot, but the object [ [1:10], [1:5] ] alone doesn't really give any indication that when plotted it should be a tree plot.

The code is modified from the code for the Julia type tree code. There, however, one wants to be able to go up as well as down the type hierarchy, so it didn't seem like there was a nice abstraction to reuse code directly.

I added an example to the README from JuliaCollections/AbstractTrees.jl#32 by @EricForgy (hope that's ok!). Here's the text form of the example, just with AbstractTrees:

julia> using AbstractTrees

julia> AbstractTrees.children(d::Dict) = [p for p in d]

julia> AbstractTrees.children(p::Pair) = AbstractTrees.children(p[2])

julia> function AbstractTrees.printnode(io::IO, p::Pair)
           str = isempty(AbstractTrees.children(p[2])) ? string(p[1], ": ", p[2]) : string(p[1], ": ")
           print(io, str)
       end

julia> d = Dict(:a => 2,:d => Dict(:b => 4,:c => "Hello"),:e => 5.0)
Dict{Symbol,Any} with 3 entries:
  :a => 2
  :d => Dict{Symbol,Any}(:b=>4,:c=>"Hello")
  :e => 5.0

julia> print_tree(d)
Dict{Symbol,Any}
├─ a: 2
├─ d: 
│  ├─ b: 4
│  └─ c: Hello
└─ e: 5.0

With this PR, plot(TreePlot(d)) is an alternative to print_tree(d) which makes an analogous graphic.

I added the graphical version of this example to the README tests, but I think Plots saves PNGs differently on my computer for some reason, because all of the images noticeably changed when I ran generate_readme_images.jl. So I suspect Travis will build the PNG differently than the one I committed and the test will fail-- maybe someone can point me to the right way to get the reference image in that case?

@JackDevine
Copy link
Member

JackDevine commented Aug 10, 2019

This looks really cool! I still haven't done a thorough review, so if you don't mind giving me some time to do so, then that would be great.

Putting the example in the README is a good idea. I am halfway through writing documentation that will end up on the JuliaPlots documentation website, so I will just incorporate your work once we merge it.

How different are the images that you produce? Do they look wrong to you when you just eyeball them, or is it VisualRegressionTests that notices a significant difference? There is a bug with 3d graphs that I am aware of, I was meaning to make an issue/fix for it.

Also note that Travis does not run generate_readme_images.jl, so if the tests pass locally, then they should pass on Travis.

EDIT
I had a better look and everything seems good to me, one minor problem is that there are a few files with no empty line at the end. The major problem is that there has been breaking changes recently (see #79), so the addition of Project.toml will not agree with semver. You can either delete the Project.toml stuff from this PR and add it in later separately or we could talk about tagging a new release.

With regards to the images, is it possible for you to show me your results for generate_readme_images.jl? You can just include images in github comments by dropping them onto the comment as you are typing.

The way that I generated the images is a little bit sloppy and will possibly get refactored when I update the documentation.

@ericphanson
Copy link
Contributor Author

ericphanson commented Aug 11, 2019

Thanks for the feedback! I will fix the newlines and update the Project.toml. I'm happy to make any other changes too.

One thing I should say, which I realize now might not be obvious since the diff for this PR includes the Project.toml change, is that this PR adds a dependency on AbstractTrees (I was just looking at the commit diff where it is very obvious so I didn't think to mention it explicitly). AbstractTrees is pretty light (~750 lines of code with no dependencies), so I hope it's ok, though I can understand reservations about adding dependencies. Anyway, I will just make the changes here too rather than removing the changes from this PR so that I can add the dependency.

Regarding the images: it's a bit strange. What happened yesterday was:

  • I ran generate_readme_images.jl, then test; tests failed locally
  • I reverted all images but the new one I added; tests passed locally (and on Travis, apparently).

Today, something different happened:

  • I ran generate_readme_images.jl, then test; tests passed locally!

Unfortunately, I didn't save the images from yesterday; the main difference was they looked higher resolution, with smaller and crisper boxes and fonts.

Here are the images generated today, which aren't that different (and pass the tests):

eph_random_labelled_graph
eph_arc_chord_diagrams
eph_AST_example
eph_julia_dict_tree
eph_julia_type_tree
eph_random_3d_graph

Edit: By the way, thanks for telling me how to upload them! That's really easy. I wanted to attach pictures yesterday but didn't know how.

@JackDevine
Copy link
Member

this PR includes the Project.toml change ... AbstractTrees

Yes, I had a look at AbstractTrees and I am fine with that dependency.

Regarding the images...

Yes that is strange, have you checked for gremlins? It is probably not your fault, the image testing code on this repo is not my finest work.

I have to admit that I have never tagged a release before. Are you sure that changing the Project.toml is all that you have to do? In any case, I feel that a new release should have approval from a couple members: @daschw @mkborregaard @asinghvi17

Changes:

The only change that is very breaking is the marker change. If I could work out how to make aliases for recipe kwargs, then I could make that change slightly less breaking.

@ericphanson
Copy link
Contributor Author

My understanding is making a new release requires adding the Project.toml, and then making a pull request to the General registry to add a new version of the package. Usually that pull request is done by installing the registrator github bot and asking it to register (see the steps here: https://github.com/JuliaRegistries/Registrator.jl/blob/master/README.md). There is also a web interface that is linked to from that readme. Then usually you tag a new version on github too (or install tagbot to do that automatically).

So adding the Project.toml is the only change needed to the contents of this repo, but it doesn’t automatically make a new version or anything— that’s achieved by changing the General registry via the registrator bot.

So this PR can be separate from the process of actually deciding to make a release or not. (And the new stuff will just be on master until a release is made).

I think also tagbot has a way for you to do patch notes which show up on the github releases page, so that list above would be great for that. I haven’t done that before though, but they talk about it here: https://github.com/JuliaRegistries/TagBot/blob/master/README.md .

@JackDevine JackDevine mentioned this pull request Aug 11, 2019
@asinghvi17
Copy link
Member

asinghvi17 commented Aug 15, 2019

@JackDevine yes, the only thing you need to do is bump the version in Project.toml, then add a comment (preferably on the latest commit, can also be in an issue) with the contents @JuliaRegistrator register. This will trigger the Registrator bot to register a new version of the package.

You can trigger Registrator with a set of release notes for TagBot to pick up, like so:

@JuliaRegistrator register

Release notes:

Check out my new features!

@JuliaRegistrator
Copy link

Pull request comments will not trigger Registrator as it is disabled. Please trying using a commit or issue comment.

@JuliaPlots JuliaPlots deleted a comment from JuliaRegistrator Aug 15, 2019
@JackDevine
Copy link
Member

JackDevine commented Aug 15, 2019 via email

@JackDevine JackDevine merged commit 5def93b into JuliaPlots:master Aug 15, 2019
@JackDevine
Copy link
Member

Thanks for this!

I am merging now.

When I get home I will fix some bugs, add some aliases for kwargs and then tag the new release!

@JackDevine JackDevine mentioned this pull request Aug 15, 2019
@ericphanson ericphanson deleted the Trees branch August 15, 2019 23:31
@mkborregaard
Copy link
Member

Great, I'm in favour of adding the dep on abstracttrees and pushing a new release.
@JackDevine @ericphanson do you think there is some possible synergy with the plotting functionality I added for phylogenetic trees (the type is also called AbstractTrees) here: EcoJulia/Phylo.jl#15 ?

@JackDevine
Copy link
Member

That sounds like an interesting point.

If I understand correctly, you are suggesting that GraphRecipes will get the ability to visualize trees in the style of EcoJulia/Phylo.jl#15 and that Phylo gets access to some of the features of GraphRecipes.

Or are you suggesting this as a one way thing?

Either way I am intrigued by the idea of getting new features "for free".

@mkborregaard
Copy link
Member

No I was just looking for whether there could be some synergies - so far these are distinct things

@JackDevine
Copy link
Member

I don't understand, can you be a little more specific? Bear in mind that I am not familiar with the code in the Phylo.jl repo.

@ericphanson ericphanson mentioned this pull request Aug 31, 2019
12 tasks
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.

5 participants