-
Notifications
You must be signed in to change notification settings - Fork 77
Added "order" argument to drawing functions. #566
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
Conversation
3ad5290
to
d8d20c1
Compare
Codecov Report
@@ Coverage Diff @@
## master #566 +/- ##
==========================================
+ Coverage 87.35% 87.36% +0.01%
==========================================
Files 22 22
Lines 16682 16696 +14
Branches 3260 3262 +2
==========================================
+ Hits 14573 14587 +14
Misses 1030 1030
Partials 1079 1079
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
This can wait until #555 is in. |
Only took a skim, but it looks good to me. |
Yeah. LGTM too. I worried that I might need a minlex_preorder function for the new SVG drawing functionality, but it turns out not, and this change should slot in quite easily. |
OK, great. Let's try to get the main SVG stuff in first and then I'll update this PR. |
@jeromekelleher the main SVG stuff is ready to merge I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Just a couple of typos in docstrings, and one code style suggestion that I'm not very attached to.
9dd7530
to
6f31ac5
Compare
Great, thanks @benjeffery. I'll rebase this and fix conflicts when @hyanwong's merge queue has settled down, no point in making his life more difficult right now. |
Talking to @hyanwong that he'd like this in first - are you happy for me to rebase and merge? |
Sure, if you don't mind resolving the merge conflicts here. I think it's a mistake though, as it'll add a bunch more conflicts for @hyanwong to resolve in his series of PRs. |
Note this changes the default order in which nodes are output in trees from the natural tree order to the minlex order which minimises the difference between adjacent trees. Closes tskit-dev#389
Add a new
order
argument to the various drawing functions that can be eithertree
(the natural order arising from the quintuply linked trees) orminlex
, the ordering from @brianzhang01'sminlex_postorder
tree traversal functions.Since this is much better, we've changed the default ordering, so that people's trees will look different now. The small chance of breakage seems worth it. As this is a breaking change, I've pinged a good few of you for review - feel free to just ignore this or "approve" if you're not bothered.
This ended up being a bit more involved than I had expected, but I think it's good now.
Closes #389