Svg corrections

[hyanwong]

Fixes #467 . I have left it as 3 separate commits in case these should be separated out, as they do different but related (and dependent) things.

[hyanwong]

No tests added, but there were none in this area before, either, AFAIK.

[hyanwong]

By the way, I haven't done as @jeromekelleher suggested and "put the id(ts) as prefix to all the values", because it is perfectly possible that someone might try to draw the same tree (or tree sequence) in a notebook but with different parameters, e.g. for height and width. We need an id for the trees in a svg tree sequence, because the id is used to position the tree along the x-axis. But if we use IDs that are the same for a given tree seq then we hit the same problem of duplicate SVG ids for what should be different drawings.

Instead, I have a small function to generate a (pretty much) unique UID suffix for the trees in a ts. I could increase the number of characters in the UID, but at the moment it generates one of ~10^14 possible identifiers, so I think the chances of clashing are low.

We needn't worry about the obscureness of the notation, I think, as it's an internal thing only. If we want to target a particular SVG drawing (tree or tree sequence) my new svg_id parameter allows a user-defined id to be passed in, which is then a handle to the correct image.

Ping @benjeffery as I think he knows about this stuff.

[codecov]

Codecov Report

Merging #555 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   87.29%   87.31%   +0.01%     
==========================================
  Files          21       21              
  Lines       16484    16502      +18     
  Branches     3237     3242       +5     
==========================================
+ Hits        14389    14408      +19     
  Misses       1029     1029              
+ Partials     1066     1065       -1     

Flag	Coverage Δ
#c_tests	88.49% <100.00%> (+0.02%)	⬆️
#python_c_tests	90.40% <100.00%> (+0.03%)	⬆️
#python_tests	99.24% <100.00%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
python/tskit/drawing.py	99.82% <100.00%> (+0.18%)	⬆️
python/tskit/trees.py	98.66% <100.00%> (ø)

---

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 48ed653...bb295af. Read the comment docs.

[hyanwong]

Just modified the last commit so that we are guaranteed all svg tree ids in a TS are unique. Overkill, but hey...

[hyanwong]

I'm being stupid here. If we place the trees in a drawn tree sequence in their own group, rather than using SVG <def>s, then we don't need a unique ID at all for each tree, and can get rid of all that messy UID stuff. This is much more sensible, as we aren't reusing the tree plots anyway (which is the point of <def>). So I've re-written the last commit to remove any need for IDs at all. I've checked and this does solve #467

[petrelharp]

Oh good! That makes much more sense.

[hyanwong]

While I'm on it, I would also like to be able to rotate the text labels by 90 degrees (see #26). It would be nice to allow this by simply changing the CSS, but it turns out to be a little complex (see here). The simplest solution would be to wrap all text elements in a <g> tag, so for each label, rather than outputting

<text class="n14" x="70.15" y="129.62">14</text>

we would use the more verbose

<g transform="translate(70.15 129.62)"><text class="n14">14</text></g>

But then we could style specific types of labels with e.g.

.labels .nodes .sample text {transform:rotate(90deg)}

I think the extra size in the svg is probably worth it. Opinions @benjeffery / @jeromekelleher ?

[jeromekelleher]

This is great, thanks @hyanwong!

Re testing, you're right there wasn't really any, but I think we should add some to verify the class structure of the output. We should be able to do this by parsing the XML output. We can put this into the verify_basic_svg function, and therefore not have to add any new tests.

The docs look great, but they're in the wrong place.

I've asked @benjeffery for a review - I'll leave it between the two of you to finish this one out as you both know more about this than I do.

[jeromekelleher]

I think the extra size in the svg is probably worth it. Opinions @benjeffery / @jeromekelleher ?

I don't think we need to worry too much about SVG size. If we have a useful feature we want, and it needs less than quadratic space then I say we add it.

[jeromekelleher]

Also, can you update the CHANGELOG to highlight this new feature (& bugfix)?

[benjeffery]

<g> are generally a good thing in SVG. I wouldn't worry about adding them. For example when doing animations changes to <g> generally give a much better frame rate than other elements.

Also def needs tests. At a bare minimum comparing the raw html string output to known good. This lets us update deps etc. with confidence.

[hyanwong]

<g> are generally a good thing in SVG. I wouldn't worry about adding them. For example when doing animations changes to <g> generally give a much better frame rate than other elements.

I agree. Do you want me to add the <g> on text as a separate PR, or roll it into this one?

Also def needs tests. At a bare minimum comparing the raw html string output to known good. This lets us update deps etc. with confidence.

happy to add a known string test. Will start coding that up now.

[benjeffery]

happy to add a known string test. Will start coding that up now.

Parsing the XML is much better as it ignores whitespace and attribute changes.
Using xmlunittest is an option:

from xmlunittest import XmlTestCase
class TestSVG(XmlTestCase):
    test_case.assertXmlEquivalentOutputs(xml_string, expected_xml_string)

[hyanwong]

@benjeffery I think this is ready to review. It seems to be failing appveyor because the appveyor config is not reading requirements/CI/requirements.txt and so is not installing xmlunittest. Not sure why Appveyor is set up like this: do you know, and also how to fix it?

[benjeffery]

@hyanwong Appveyor installs from python\requirements\conda-minimal.txt

[hyanwong]

I'm also hesitant about documenting the exact pixel-sizes of the layouts,

Are we committing to keeping those sizes though? Could we not say “Currently the defaults are xxx but this may be subject to change in future releases”?

[jeromekelleher]

Are we committing to keeping those sizes though? Could we not say “Currently the defaults are xxx but this may be subject to change in future releases”?

No, but documentation like this has a nasty habit of going out of date because we forget about it. I wouldn't put in any details that we aren't confident are stable. It's not an important detail that users need to know.

[hyanwong]

This is all done. If you are happy @jeromekelleher and @benjeffery, I'll squash the last commit which addresses the last concerns into the one before it, check if it needs rebaseing, and then someone can merge.

[benjeffery]

A thought on customising I had today (not for this PR) - one way to do it would be to have callback params that allow functions to be specified that return HTML for nodes and edges etc. The callback would be called with the node/edge/mutation data and the default HTML entity. The callback then either makes a new HTML entity, or modifies the default and returns it.

[hyanwong]

An observation that any styling passed in to one SVG on a Jupyter notebook will affect all the trees in the notebook (as per a comment I saw: "scoped CSS has been removed from the specs, so any <style> tags in the document will be hoisted to the global scope"). The only way to stop this is to given an ID to the SVG and target the style at that SVG. We should probably give an example of this in the docs, as it's a bit unexpected to the uninitiated.

[benjeffery]

@hyanwong A quick added sentence on the style param docs should do it.

[jeromekelleher]

This is great, thanks @hyanwong, let's get this one merged in ASAP. Looks like we're basically there.

[benjeffery]

Yep, think it is just the doc mention of style hoisting.

[hyanwong]

Added note about styles and squashed the PR changes into the last commit. Should be OK to merge I guess @benjeffery

[benjeffery]

@hyanwong I see you've just made some changes, is this ready for a once over as am keen to merge this so the others can proceed?

[hyanwong]

It was probably accidental. I did squash the last PR corrections into the previous commit, but it should otherwise be the same. I think I'm confused as to what I did. Can you pinpoint any changes?

[hyanwong]

It's frustrating working on 5 interrelated PRs at once. Especially as I can't always do them on the same computer at the moment. So I never remember which local branches are pushed, etc etc. I think I've whinged to you before that I can't really get git to suit my workflow (which tends to be to not complete anything if I can see something shiny elsewhere)

[hyanwong]

@benjeffery it's definitely ready for a final look, and hopefully a merge, though. I did have to rebase too, as Daniel & Sunack's changes went in before this was sorted.

[benjeffery]

Ok, I'll give it the once over. Are you happy for me to make any small changes myself as might be late by the time I am done? (will leave anything big till the morning, although don't expect there to be anything big at all!)

Lets talk about git workflow on a call - would be good if we can make this easier for you.

[hyanwong]

Wow, you're working late. I had gone to bed. Sorry! Thanks for merging