Skip to content

Conversation

@hyanwong
Copy link
Member

@hyanwong hyanwong commented Apr 28, 2020

To be considered after #555 (on which this is based - the only new stuff is in the last commit in this PR). New (default) behaviour is equivalent to ts.draw_svg(x_scale="physical"), old behaviour is gained by ts.draw_svg(x_scale="treewise"). x_scale="physical" produces tree sequence SVG plots like the one below. There are some hard-coded margin values etc that could probably be improved, and suggestions as to better parameter nomenclature welcome.

Screenshot 2020-04-28 at 22 49 05

@hyanwong hyanwong force-pushed the background-svg-shading branch 3 times, most recently from 7c5764f to 502dfab Compare April 29, 2020 00:32
@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #563 into master will decrease coverage by 0.02%.
The diff coverage is 77.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
- Coverage   87.30%   87.27%   -0.03%     
==========================================
  Files          22       22              
  Lines       16718    16735      +17     
  Branches     3267     3272       +5     
==========================================
+ Hits        14595    14605      +10     
- Misses       1040     1043       +3     
- Partials     1083     1087       +4     
Flag Coverage Δ
#c_tests 88.47% <77.41%> (-0.05%) ⬇️
#python_c_tests 90.37% <77.41%> (-0.07%) ⬇️
#python_tests ?
Impacted Files Coverage Δ
python/tskit/trees.py 98.18% <ø> (ø)
python/tskit/drawing.py 98.67% <77.41%> (-1.16%) ⬇️

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 7a5376d...5eac898. Read the comment docs.

@jeromekelleher
Copy link
Member

Beautiful! I think the parameter names are just right.

@hyanwong
Copy link
Member Author

Beautiful! I think the parameter names are just right.

Thanks @jeromekelleher. I guess the only missing thing is a check on the two allowed values (I was waiting until this seemed like a good nomenclature). I coded x_scale as a text string in case we want to leave the option to scale e.g. by genetic distance in the future.

@jeromekelleher
Copy link
Member

Sound good. Let's look at it again when the stuff it depends on is merged.

@benjeffery benjeffery force-pushed the background-svg-shading branch 3 times, most recently from 0de5652 to d60bacd Compare May 7, 2020 23:39
@benjeffery
Copy link
Member

This one is the next SVG PR to go in. It's rebased to master, just needs some coverage of the "tree-wise" path and an error thrown when a bad x_scale is passed.

@jeromekelleher
Copy link
Member

Yes, let's get this one merged ASAP, just needs some small updates. The pattern used for other string constants like this has been

x_scale = check_xscale(x_scale)

where check_xscale() decides the defaults, etc. There's a bunch of these functions at the top of the file.

@hyanwong
Copy link
Member Author

There's a bunch of these functions at the top of the file.

Yep, perfect. Do you want me to do this @benjeffery, or can you do it (I think you said you might!)?

@benjeffery
Copy link
Member

I can fix it up, squash and rebase.

@benjeffery benjeffery force-pushed the background-svg-shading branch from d60bacd to 0f8369a Compare May 11, 2020 13:25
@benjeffery benjeffery force-pushed the background-svg-shading branch from 0f8369a to 5eac898 Compare May 11, 2020 13:33
@benjeffery
Copy link
Member

I've added the extra coverage and x_scale check. Unless you want a final review @jeromekelleher @hyanwong this is ready to merge.

@jeromekelleher
Copy link
Member

Merge away I'd say @benjeffery

@benjeffery
Copy link
Member

Hmm, travis is AWOL. Checking.

@jeromekelleher
Copy link
Member

Feel free to just merge stuff if CI has passed on Circle and we know that it'll pass on the others (i.e., it's already pass n times before)

@benjeffery benjeffery merged commit 971b5b6 into tskit-dev:master May 11, 2020
@hyanwong hyanwong deleted the background-svg-shading branch May 12, 2020 14:48
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.

3 participants