Ignore hidden nodes in domain

[victorlin]

Description of proposed changes

draft; see linked issue for context

Related issue(s)

• Closes Long, hidden internal branches result in empty space #1950

Checklist

• Checks pass
• If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
• (to be done by a Nextstrain team member) Create preview PRs on downstream repositories.

[jameshadfield]

Tested using https://nextstrain-s-nextstrain-wqnhof.herokuapp.com/community/victorlin/nextstrain-test@1d26348/ebola/all-outbreaks and it works well. Note that radial & unrooted layouts aren't improved by this PR - up to you whether you want to address them here or leave them unchanged.

[jameshadfield]

What's the intention of this change? Is the change meant for non-rectangular trees only (if so, I'd add a this.layout!=='rect' conditional)? Or were you intending

    if (this.layout==="rect" && (this.zoomNode.n.arrayIdx!==0 || getBranchVisibility(this.zoomNode) === "hidden"))

which would add LHS padding when the root was hidden.

P.S. This entire aspect of Auspice is very ad-hoc and could use broader improvements.

[victorlin]

It's meant for rectangular trees only, so yes to your code snippet. I forgot to test other layouts, will take a look to see if it's easy.

[victorlin]

Updated the condition to your code snippet:

auspice/src/components/tree/phyloTree/layouts.ts

Line 402 in 29b03a9

if (this.layout==="rect" && (this.zoomNode.n.arrayIdx!==0 || getBranchVisibility(this.zoomNode) === "hidden")) {

[victorlin]

Updated the condition to your code snippet:

auspice/src/components/tree/phyloTree/layouts.ts

Line 402 in 29b03a9

if (this.layout==="rect" && (this.zoomNode.n.arrayIdx!==0 || getBranchVisibility(this.zoomNode) === "hidden")) {

[victorlin]

Started addressing radial layout in 29b03a9 but still needs some work.

[victorlin]

@jameshadfield could you help with me radial and unrooted layouts if you get the chance? I don't quite understand how they work, but would like to sort them out in this PR if it doesn't require too many code changes.

I've pushed what I have so far. Rect works well, radial is half-baked, and unrooted looks the same as before.

[jameshadfield]

Revisiting this afresh with new eyes after a couple of months.

If we take a dataset such as your ebola all-outbreaks one, the aim of this PR is to take the current view (screenshot) and instead restrict the x-domain to (roughly) the dashed box:

Focusing on the west-african outbreak, we can see the subtlety involved in #1950 which stated "Hidden nodes are considered in calculation of X domain". The hidden node contributes to the x-domain in the sense that it defines the origin value for the visible branch. (We don't want to hide NODE_0000205 as we want to preserve the tee (vertical branch) at around x=2007.

This PR's implementation (for rect trees) calculates the domain by excluding hidden nodes, i.e. considering the west-african subtree the domain would start at around 2007.5, and then 5% of the domain span is added to the LHS to accomodate branch labels. This seems appropriate but I'd go further and change the rendering of branches which are children of hidden nodes so that they no longer render their stem, which this patch will do:

diff --git a/src/components/tree/phyloTree/renderers.ts b/src/components/tree/phyloTree/renderers.ts
index 35128add..2d3503bc 100644
--- a/src/components/tree/phyloTree/renderers.ts
+++ b/src/components/tree/phyloTree/renderers.ts
@@ -330,7 +330,11 @@ export const drawBranches = function drawBranches(this: PhyloTreeType): void {
     })
     .style("stroke-linecap", "round")
     .style("stroke-width", (d) => d['stroke-width'] || params.branchStrokeWidth)
-    .style("visibility", getBranchVisibility)
+    .style("visibility", (d) => {
+      // hide the stem between a hidden parent and a visible child
+      if (getBranchVisibility(d.n.parent.shell)==='hidden') return 'hidden'
+      return getBranchVisibility(d)
+    })
     .style("cursor", (d) => d.visibility === NODE_VISIBLE ? "pointer" : "default")
     .style("pointer-events", "auto")
     .on("mouseover", this.callbacks.onBranchHover)

which results in a tree like so:

Note: this patch is a proof-of-principle. There are bugs when you interact with the tree (zooming, layout changes) that we need to track down. Alternatively you can filter the nodes (instead of hiding the stems), which may or may not be better.

---

In radial trees, the above patch improves things a bunch but the tee-paths are still incorrectly calculated, most obviously seen on branches which sweep a large $\theta$

This can be fixed by updating the offset used within the path (arc) generation,

diff --git a/src/components/tree/phyloTree/layouts.ts b/src/components/tree/phyloTree/layouts.ts
index 07e4141b..873e7815 100644
--- a/src/components/tree/phyloTree/layouts.ts
+++ b/src/components/tree/phyloTree/layouts.ts
@@ -513,7 +522,8 @@ export const mapToScreen = function mapToScreen(this: PhyloTreeType): void {
       }
     });
   } else if (this.layout==="radial") {
-    const offset = this.nodes[0].depth;
+    const visibleNodes = this.nodes.filter((d) => getBranchVisibility(d) === "visible");
+    const offset = Math.min(...visibleNodes.map((d) => d.depth));
     const stem_offset_radial = this.nodes.map((d) => {return (0.5*(stemParent(d.n).shell["stroke-width"] - d["stroke-width"]) || 0.0);});
     this.nodes.forEach((d, i) => {
       d.branch =[

---

For unrooted layouts I would be very careful with changes to the (x) domain. There is an inherent conflict here as trees are always rooted as per the JSON format, and hidden nodes somewhat depend on the tree being rooted, but we have an unrooted layout. Specifically, ignoring basal nodes in unrooted layouts is going to shift the (visible) subclades closer together thus making them seem genetically/temporally more closely related than they are. My advice would be to not change them as part of this PR (beyond hiding branches, as discussed above).

---

A few other things I noticed while exploring this - all of which are beyond this PR, so I'll turn them into issues afterwards:

1. In rectangular trees, the following code block is why the min x-domain is left of the Yambuku outbreak, which is fine, but we can be much smarter about how much space to leave such that any branch labels are correctly rendered. One day we should find some time to improve the rendering of branch labels, tip labels, streamtree labels etc.

    /* In rectangular mode, if the tree has been zoomed, leave some room to display the (clade's) root branch */
    if (this.layout==="rect" && (this.zoomNode.n.arrayIdx!==0 || getBranchVisibility(this.zoomNode) === "hidden")) {
      minX -= (maxX-minX)/20; // 5%
    }

2. Zooming out ("-" button, or clicking in-view root node) traverses the tree structure as normal, but it doesn't consider the hidden property. So when we zoom out from the Kikwit outbreak we should go back to seeing all outbreaks, but we don't (RHS):

3. The getBranchVisibility is getting used more frequently now and we're doing unnecessary string equality checking. It'd be more ergonomic to add a branchVisible(...): boolen function