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

Single tip tree invisible in circular layout #429

Closed
gibsramen opened this issue Oct 16, 2020 · 1 comment · Fixed by #430
Closed

Single tip tree invisible in circular layout #429

gibsramen opened this issue Oct 16, 2020 · 1 comment · Fixed by #430
Labels

Comments

@gibsramen
Copy link
Collaborator

Came up when I was testing the shearing to feature metadata addition #428. I used the moving pictures dataset with a feature metadata file with only one entry.

image

When I used this taxonomy file and sheared the tree to only include this single tip - the circular layout did not display anything. The unrooted and rectangular layouts display a visible vertical and horizontal line respectively.

ezgif com-gif-maker (1)

@fedarko fedarko added the bug label Oct 16, 2020
@fedarko
Copy link
Collaborator

fedarko commented Oct 16, 2020

Can confirm that this is a problem in the master branch of Empress. It looks like the problem is coming from the scaling stuff within the circular layout:

var widthScale = width / (maxX - minX);
var heightScale = height / (maxY - minY);
var scaleFactor = Math.max(widthScale, heightScale);
// Go over the tree (in postorder, but order doesn't really matter
// for this) to determine arc positions for non-root internal nodes.
// I think determining arc positions could be included in the above for
// loop...
for (i = 1; i < tree.size; i++) {
if (normalize) {
x0[i] *= scaleFactor;
x1[i] *= scaleFactor;
y0[i] *= scaleFactor;
y1[i] *= scaleFactor;
}

The circular layout should show up in this case as a horizontal line from the root to the single tip, the same as with the rectangular layout. What's causing this problem is that minY and maxY are both 0, so heightScale gets set to 4020 / 0 = Infinity, which is then used to "scale" all of the coordinates in the tree (which explodes things).

This problem is handled in the rectangular layout by explicitly checking if the max height is > 0 before trying to do y scaling:

// If the tree is a straight line (i.e. all internal nodes have exactly
// one child), every node in the tree will have a y-coordinate of 0. In
// this case, this scaling factor won't matter, since it'll get
// multiplied by 0 for every node.
var yScalingFactor = 1;
// Having a max_height of 0 could actually happen, in the funky case
// where the entire tree is a straight line (e.g. A -> B -> C). In
// this case our "rectangular layout" drawing places all nodes on
// the same y-coordinate (0), resulting in max_height = 0.
// ... So, that's why we only do y-scaling if this *isn't* the case.
if (maxHeight > 0) {
yScalingFactor = height / maxHeight;
}
if (normalize) {
for (i = 1; i <= tree.size; i++) {
xCoord[i] *= xScalingFactor;
yCoord[i] *= yScalingFactor;
}
}

It should be possible to handle this similarly here (maybe by just setting scaleFactor to widthScale, since the tree should be guaranteed to take up some x space).

It's also worth noting that this case is actually accounted for in the JS tests, but this test sets the normalize parameter to false. A solution to this problem should include adding at least a test that verifies that single-tip trees work properly with normalization.

fedarko added a commit to fedarko/empress that referenced this issue Oct 16, 2020
fedarko added a commit to fedarko/empress that referenced this issue Oct 16, 2020
fedarko added a commit to fedarko/empress that referenced this issue Oct 17, 2020
kwcantrell pushed a commit that referenced this issue Nov 10, 2020
…tAngle circular layout parameter (#430)

* BUG: Fix #429

* TST: update tests re: no circ layout start angle

these tests were annoying to maintain anyway :ppp

* TST/BUG: fix x circscaling for 1tip case; tst #429

* BUG/TST: epsilon for dy in circ normalization; tst

Added a new circ layout test, and changed up the approx deep equal
multi stuff a bit

* STY: alias UFT.approxDeepEqualMulti

* Add straight-line unrooted layout test

* MNT/TST: abstract scale fact comp to new func

uses max instead of min, so had to upd8 unrooted test

* add sf comp direct unit test for error case

* add additional sf test

* Doc epsilon some more in sf comp func

* TST: add finalish scale factor func tests #429

* doc rect layout and sf func

* STY: prettify tests

* TST/DOC: Clarify root length thing

Addresses @kwcantrell's comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants