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

Refactor text drawing #242

Merged
merged 7 commits into from
Jun 26, 2019

Conversation

jeromekelleher
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

Merging #242 into master will increase coverage by 0.11%.
The diff coverage is 97.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   86.66%   86.77%   +0.11%     
==========================================
  Files          17       17              
  Lines        9446     9508      +62     
  Branches     1712     1727      +15     
==========================================
+ Hits         8186     8251      +65     
+ Misses        750      749       -1     
+ Partials      510      508       -2
Flag Coverage Δ
#c_tests 86.77% <97.23%> (+0.11%) ⬆️
#python_tests 98.39% <97.23%> (+0.14%) ⬆️
Impacted Files Coverage Δ
python/tskit/trees.py 98.71% <100%> (ø) ⬆️
python/tskit/drawing.py 95.8% <97.16%> (+0.89%) ⬆️

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 c18ac17...feefe08. Read the comment docs.

@jeromekelleher
Copy link
Member Author

Pinging @petrelharp, @hyanwong, @brianzhang01, @awohns, @gtsambos as tree-drawers.

There's some significant updates to the text drawing API here, and it would be good to get some feedback. Firstly, we have nice rendering of tree sequences: hooray!

4.94┊    18           ┊                 ┊                 ┊                 ┊                 ┊
    ┊  ┏━━┻━━┓        ┊                 ┊                 ┊                 ┊                 ┊
3.30┊  ┃     ┃        ┊    17           ┊    17           ┊      17         ┊        17       ┊
    ┊  ┃     ┃        ┊  ┏━━┻━━┓        ┊  ┏━━┻━━┓        ┊   ┏━━━┻━━━┓     ┊     ┏━━━┻━━━┓   ┊
2.03┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃    16        ┊  16       ┃     ┊     ┃       ┃   ┊
    ┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃  ┏━━┻━━┓     ┊ ┏━┻┓      ┃     ┊     ┃       ┃   ┊
1.03┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃  ┃     ┃     ┊ ┃  ┃     15     ┊    15       ┃   ┊
    ┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃  ┃     ┃     ┊ ┃  ┃   ┏━━┻━┓   ┊  ┏━━┻━┓     ┃   ┊
0.86┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃  ┃     ┃     ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃    14   ┊
    ┊  ┃     ┃        ┊  ┃     ┃        ┊  ┃  ┃     ┃     ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃   ┏━┻┓  ┊
0.40┊  ┃    13        ┊  ┃    13        ┊  ┃  ┃     ┃     ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃   ┃  ┃  ┊
    ┊  ┃  ┏━━┻━━┓     ┊  ┃  ┏━━┻━━┓     ┊  ┃  ┃     ┃     ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃   ┃  ┃  ┊
0.12┊  ┃  ┃    12     ┊  ┃  ┃    12     ┊  ┃  ┃    12     ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃   ┃  ┃  ┊
    ┊  ┃  ┃   ┏━┻━━┓  ┊  ┃  ┃   ┏━┻━━┓  ┊  ┃  ┃   ┏━┻━━┓  ┊ ┃  ┃   ┃    ┃   ┊  ┃    ┃   ┃  ┃  ┊
0.10┊  ┃  ┃   ┃   11  ┊  ┃  ┃   ┃   11  ┊  ┃  ┃   ┃   11  ┊ ┃ 11   ┃    ┃   ┊  ┃    ┃   ┃ 11  ┊
    ┊  ┃  ┃   ┃   ┏┻┓ ┊  ┃  ┃   ┃   ┏┻┓ ┊  ┃  ┃   ┃   ┏┻┓ ┊ ┃ ┏┻┓  ┃    ┃   ┊  ┃    ┃   ┃ ┏┻┓ ┊
0.09┊  ┃  ┃  10   ┃ ┃ ┊  ┃  ┃  10   ┃ ┃ ┊  ┃  ┃  10   ┃ ┃ ┊ ┃ ┃ ┃  ┃   10   ┊  ┃   10   ┃ ┃ ┃ ┊
    ┊  ┃  ┃ ┏━┻┓  ┃ ┃ ┊  ┃  ┃ ┏━┻┓  ┃ ┃ ┊  ┃  ┃ ┏━┻┓  ┃ ┃ ┊ ┃ ┃ ┃  ┃  ┏━┻┓  ┊  ┃  ┏━┻┓  ┃ ┃ ┃ ┊
0.02┊  9  ┃ ┃  ┃  ┃ ┃ ┊  9  ┃ ┃  ┃  ┃ ┃ ┊  9  ┃ ┃  ┃  ┃ ┃ ┊ ┃ ┃ ┃  9  ┃  ┃  ┊  9  ┃  ┃  ┃ ┃ ┃ ┊
    ┊ ┏┻┓ ┃ ┃  ┃  ┃ ┃ ┊ ┏┻┓ ┃ ┃  ┃  ┃ ┃ ┊ ┏┻┓ ┃ ┃  ┃  ┃ ┃ ┊ ┃ ┃ ┃ ┏┻┓ ┃  ┃  ┊ ┏┻┓ ┃  ┃  ┃ ┃ ┃ ┊
0.01┊ ┃ ┃ ┃ ┃  8  ┃ ┃ ┊ ┃ ┃ ┃ ┃  8  ┃ ┃ ┊ ┃ ┃ ┃ ┃  8  ┃ ┃ ┊ ┃ ┃ ┃ ┃ ┃ ┃  8  ┊ ┃ ┃ ┃  8  ┃ ┃ ┃ ┊
    ┊ ┃ ┃ ┃ ┃ ┏┻┓ ┃ ┃ ┊ ┃ ┃ ┃ ┃ ┏┻┓ ┃ ┃ ┊ ┃ ┃ ┃ ┃ ┏┻┓ ┃ ┃ ┊ ┃ ┃ ┃ ┃ ┃ ┃ ┏┻┓ ┊ ┃ ┃ ┃ ┏┻┓ ┃ ┃ ┃ ┊
0.00┊ 4 5 3 0 1 2 6 7 ┊ 4 5 3 0 1 2 6 7 ┊ 4 5 3 0 1 2 6 7 ┊ 3 6 7 4 5 0 1 2 ┊ 4 5 0 1 2 3 6 7 ┊
  0.00              0.14              0.22              0.76              0.84              1.00

The drawing is done by the drawing.TextTreeSequence class, which delegates most of the work to the TextTree class. The format of the time and span axis labels can be controlled using the time_label_format and position_label_format respectively. I think I've taken care of all the corner cases, and this feature is pretty solid now.

There's also a preliminary pass at drawing left-to-right trees using the orientation='left' option:

 ┏━━━━━━━━━━2
 ┃
8┫        ┏━0
 ┃  ┏━━━━5┫
 ┃  ┃     ┗━1
 ┗━7┫
    ┃  ┏━━━━3
    ┗━6┫
       ┗━━━━4

This basically works, but I still need to figure out how to create space for the labels along the branches. The reason I want to sort this out now is that I want to finalise the draw_text API, and to do this I need to understand the differences that will happen when we have horizontally rendered trees like this (for example, the time scales are much more complicated now because of the need to fit labels in the branches).

Questions:

  1. Does this look good visually? Any ideas for tweaks?
  2. Does the API seem sensible?
  3. Can you think of any obvious extensions that won't fit into the API that's outlined here?

The plan would then be to mirror the structure of this API in the draw_svg code. SVG is a lot easier because you don't have a to worry about allocating space for things...

@gtsambos
Copy link
Member

There's also a preliminary pass at drawing left-to-right trees using the orientation='left' option:

Yay, looks good Jerome! I was literally just thinking today that it would be useful to be able to draw rotated trees, as I've come up with some ancestry-related stepwise functions that are best conceptualised with time on the horizontal axis (I will show you at our next f2f meeting).

I think it would be more natural for these trees to be the other way around (ie. leaves on the left, root on the right). If so, then when you plot a sequence of left-aligned trees, the horizontal time axis will be the same for each tree. As it is, the same leaves on different trees will have different horizontal coordinates, which is a bit confusing given that they are all at time = 0. I also think this would be a bit more intuitive, since bigger (backwards-in)-time values would then be further to the right, just like on a number line.

@jeromekelleher
Copy link
Member Author

As it is, the same leaves on different trees will have different horizontal coordinates, which is a bit confusing given that they are all at time = 0. I also think this would be a bit more intuitive, since bigger (backwards-in)-time values would then be further to the right, just like on a number line.

We'd definitely make sure that the nodes with the same time have the same x coordinates, which is true of the version above (but maybe your browser isn't rendering unicode correctly? This is a long-running issue, see #189).

Having alignment="right" as well as "left" would also be a logical option. Shouldn't be that hard to do.

@jeromekelleher
Copy link
Member Author

OK, this is looking like it's the right way to do things so I'm going to merge it. I'll open some issues tracking where we want to go with the tree drawing APIs once the SVG code has been cleaned up as well.

@jeromekelleher jeromekelleher merged commit 4ed604d into tskit-dev:master Jun 26, 2019
@jeromekelleher jeromekelleher deleted the refactor-text-drawing branch June 26, 2019 16:45
@petrelharp
Copy link
Contributor

this is wonderful.

@brianzhang01
Copy link
Member

So I'm on tskit 0.2.1 now and had my first spin with the new drawing functionality. However, I needed to call ts.draw_text() and ts.first().draw_text(), I couldn't just use ts.first().draw() to get the orientaiton = "left" option. Furthermore, neither of the draw_text() methods is currently documented here. Was this intended until we further clarify / finalize the API?

@jeromekelleher
Copy link
Member Author

The draw_text() and draw_svg() versions are experimental and aren't documented for the moment. I was planning to use them within the developer community for a bit before finalising the APIs and documenting fully. The long-term vision at the moment is that we'll keep the current Tree.draw() interface as a relatively simple interface and let draw_text and draw_svg be more tightly focused on text and SVG. In particular, we want to document the structure of the output SVG in draw_svg so that users can manipulate it for (e.g.) animations.

Probably we'll add the orientation argument to draw() at some point, but we'll need to implement it in draw_svg first.

Thanks for trying this stuff out --- please open issues if you have any feedback or spot any problems!

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.

4 participants