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 Tree.draw_tikz() to output tex/tikz drawing code. #798

Closed
wants to merge 3 commits into from

Conversation

grahamgower
Copy link
Member

No description provided.

@grahamgower grahamgower marked this pull request as draft August 27, 2020 07:07
@grahamgower
Copy link
Member Author

Ping @gtsambos.

@gtsambos gtsambos self-assigned this Aug 27, 2020
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks good to me @grahamgower, but I just had a quick scan.

python/tskit/drawing.py Outdated Show resolved Hide resolved
@gtsambos
Copy link
Member

fyi @grahamgower, I'm forking this now so I can play around with getting mutations in here

@gtsambos
Copy link
Member

Hi @grahamgower, I've created a PR against your fork (let me know if you have trouble with this). The additional code puts mutations on the plots, like this:

image

So far the code assumes mutation times aren't known -- but the case when they are shouldn't be difficult either, will look into it later this weekend

@grahamgower
Copy link
Member Author

Thanks, that looks great!

@grahamgower
Copy link
Member Author

grahamgower commented Aug 28, 2020

and, TreeSequences...
s

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #798 (faaac9c) into main (a7413ff) will decrease coverage by 6.37%.
The diff coverage is 8.87%.

❗ Current head faaac9c differs from pull request most recent head c7d8b68. Consider uploading reports for the commit c7d8b68 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
- Coverage   93.60%   87.22%   -6.38%     
==========================================
  Files          27       24       -3     
  Lines       23035    19558    -3477     
  Branches     1079     3686    +2607     
==========================================
- Hits        21561    17060    -4501     
+ Misses       1440     1403      -37     
- Partials       34     1095    +1061     
Flag Coverage Δ
c-tests ?
c_tests 85.33% <ø> (?)
lwt-tests ?
python-c-tests ?
python-tests ?
python_c_tests 89.13% <8.87%> (?)
python_tests 96.44% <8.87%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/tskit/drawing.py 86.91% <8.73%> (-12.51%) ⬇️
python/tskit/trees.py 96.93% <9.52%> (-1.02%) ⬇️
c/tskit/stats.c 69.14% <0.00%> (-16.02%) ⬇️
python/_tskitmodule.c 82.99% <0.00%> (-8.86%) ⬇️
c/tskit/tables.c 81.27% <0.00%> (-8.63%) ⬇️
c/tskit/haplotype_matching.c 90.42% <0.00%> (-4.64%) ⬇️
c/tskit/convert.c 87.50% <0.00%> (-4.55%) ⬇️
c/tskit/trees.c 90.45% <0.00%> (-4.41%) ⬇️
c/tskit/genotypes.c 90.50% <0.00%> (-3.40%) ⬇️
c/tskit/core.c 94.08% <0.00%> (-3.01%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7413ff...c7d8b68. Read the comment docs.

@gtsambos
Copy link
Member

very nice 👀 I'd like to get a handle on the TreeSequence-wide draw_tikz method too, perhaps I could do some minor fixups on that to help me learn my way around it? I'm thinking scaling all the trees according to the max root time, and perhaps also adding axis labels

@grahamgower
Copy link
Member Author

Yeah, adding the scaling and axes would be really useful. Just give me a moment, I've changed to use a \pic, and need to commit that.

@grahamgower
Copy link
Member Author

Ok, have at it @gtsambos! Now we can do silly things like this:

import msprime
ts = msprime.simulate(10, recombination_rate=1, mutation_rate=1, random_seed=1)
ts.draw_tikz(
    "/tmp/s.tex",
    tree_height_scale="rank",
    standalone=True,
    preamble="\\usetikzlibrary{3d, shapes.geometric}",
    style="""%
        every pic/.append style={canvas is zy plane at x},
        mutationlabels/.append style = {
            opacity=0,
        },
        mutations/.append style = {
            star,
            star point ratio=2.5,
        },\
        """
)

s1

@grahamgower
Copy link
Member Author

With bigger sample sizes (n=1000, with 19 trees) I get: ! TeX capacity exceeded, sorry [main memory size=5000000]. Maybe we can structure things differently to improve this, but it's likely we're not going to be able to scale as well as for svg.

@grahamgower
Copy link
Member Author

The solution for large drawings is to compile with lualatex, rather than pdflatex. It took a minute or two, but was actually quicker than calling draw_svg(). I guess this is beyond the limit of what's sensible.

msprime.simulate(1000, recombination_rate=1, mutation_rate=1, random_seed=1)
$ ls -lh /tmp/s.{tex,pdf,svg}
-rw-r--r-- 1 grg grg 4,5M Aug 29 10:15 /tmp/s.pdf
-rw-r--r-- 1 grg grg  16M Aug 29 10:23 /tmp/s.svg
-rw-r--r-- 1 grg grg 1,5M Aug 29 10:14 /tmp/s.tex

@gtsambos
Copy link
Member

With bigger sample sizes (n=1000, with 19 trees) I get: ! TeX capacity exceeded, sorry [main memory size=5000000]. Maybe we can structure things differently to improve this, but it's likely we're not going to be able to scale as well as for svg.

The solution for large drawings is to compile with lualatex, rather than pdflatex. It took a minute or two, but was actually quicker than calling draw_svg(). I guess this is beyond the limit of what's sensible.

I wouldn't be too worried about this -- I doubt people will be using draw_tikz on this kind of scale.

@jeromekelleher
Copy link
Member

Yeah, I'm 100% not worried about rendering large trees within tex. This is fundamentally for printed page output, so if it's too big to go on a printed page we don't care about it.

@gtsambos gtsambos mentioned this pull request Sep 5, 2020
@benjeffery benjeffery changed the base branch from master to main September 28, 2020 12:12
@benjeffery
Copy link
Member

@grahamgower does this need tests to be mergable? Or are there still open questions about the implementation?

@grahamgower
Copy link
Member Author

Probably tests and docs are the biggest things outstanding. And making sure the API is consistent enough with the other drawing functions would be a good idea.

@grahamgower
Copy link
Member Author

Rebased. I'll take another look though this in the coming week.

@jeromekelleher
Copy link
Member

jeromekelleher commented Jul 26, 2021

This is really useful and a great addition, but I'm gone off the idea of incorporating it into tskit. Our experience with the SVG viz is that it quickly turns into a lot of code, and it's quite hard to test. In this case, we'd need to pull in a tex engine to CI and for all of our client side dependencies. Keeping consistency between the various viz APIs is a pain also.

@benjeffery, I'm guessing you're in a similar mind?

So, I suggest spinning it out into an independent project? tstikz or something?

@benjeffery
Copy link
Member

So, I suggest spinning it out into an independent project? tstikz or something?

I think this is probably the right thing to do, and that maybe even the svg code is hefty enough not be in tskit, but that is another discussion!

@hyanwong
Copy link
Member

hyanwong commented Dec 17, 2021

Does this belong in https://github.com/tskit-dev/tsviz ? Perhaps we could move the PR over there and eventually merge it?

@benjeffery
Copy link
Member

Does this belong in https://github.com/tskit-dev/tsviz ? Perhaps we could move the PR over there and eventually merge it?

Sounds good to me!

@benjeffery
Copy link
Member

Closing this as we decided to move it to tsviz - have opened tskit-dev/tsviz#9 so this PR code doesn't get lost.

@benjeffery benjeffery closed this Sep 23, 2024
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