Update filters to hide ancestral nodes before last common ancestor

[joverlee521]

This PR closes #1240.

Before this commit, filters determined visibility by:

1. Finding all terminal nodes in view window that match the filters and
   mark them as visible.
2. Recursively marking all parent nodes of these terminal nodes visible,
   all the way up to the root of the tree.

This commit adds an additional step to hide ancestral nodes before
the last common ancestor of visible terminal nodes. Starting from
the root of the tree, recursively hide each node if it does not have
more than one child node that is visible.

[joverlee521]

@jameshadfield What's the expected behavior if the filter selects a single sample?

The view of a single sample with this change doesn't look great to me:

[emmahodcroft]

Good catch Jover! You make a good point. Perhaps if just one node is selected we want to retain the old behaviour, just so it doesn't look so weird. Can we have it both ways, or would that be quite difficult? Also curious to hear if James has even a better idea here!

[jameshadfield]

Thanks @joverlee521 -- this looks great.

The view of a single sample with this change doesn't look great to me:

I agree, especially as we have no way of showing a tip (circle) without its corresponding branch. However I don't think the solution is to make ancestral nodes thicker (as we did previously) as I think that's harder to interpret looking at the tree and results in transmission lines on the map which are really confusing. My preferred solution involves some form of zooming and is best implemented via #1132 or similar.

---

The only confusing aspect of the current implementation is that we hide nodes above the CA of the selected tips in the context of the current viewport, rather than the true CA. I tried to quickly modify the PR to show this but it got a bit complicated (patch attached below, but would need tidying up). Zooming into the branch shown in (A), the current PR (B) recomputes a CA relative to the two inView tips, whereas I argue it should be the "true" CA, which is offscreen and thus the branches should be as in (C). @trvrb @rneher what are your views here?

Patch:

diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js
index 4cf121d9..b78ae380 100644
--- a/src/actions/recomputeReduxState.js
+++ b/src/actions/recomputeReduxState.js
@@ -550,7 +550,8 @@ const modifyTreeStateVisAndBranchThickness = (oldState, zoomSelected, controlsSt
   const visAndThicknessData = calculateVisiblityAndBranchThickness(
     oldState,
     controlsState,
-    {dateMinNumeric: controlsState.dateMinNumeric, dateMaxNumeric: controlsState.dateMaxNumeric}
+    {dateMinNumeric: controlsState.dateMinNumeric, dateMaxNumeric: controlsState.dateMaxNumeric},
+    newIdxRoot
   );
 
   const newState = Object.assign({}, oldState, visAndThicknessData);
diff --git a/src/actions/tree.js b/src/actions/tree.js
index 22accbc0..e3cd1e82 100644
--- a/src/actions/tree.js
+++ b/src/actions/tree.js
@@ -65,7 +65,8 @@ export const updateVisibleTipsAndBranchThicknesses = (
     const data = calculateVisiblityAndBranchThickness(
       tree,
       controls,
-      {dateMinNumeric: controls.dateMinNumeric, dateMaxNumeric: controls.dateMaxNumeric}
+      {dateMinNumeric: controls.dateMinNumeric, dateMaxNumeric: controls.dateMaxNumeric},
+      rootIdxTree1
     );
     const dispatchObj = {
       type: types.UPDATE_VISIBILITY_AND_BRANCH_THICKNESS,
@@ -85,6 +86,7 @@ export const updateVisibleTipsAndBranchThicknesses = (
         treeToo,
         controls,
         {dateMinNumeric: controls.dateMinNumeric, dateMaxNumeric: controls.dateMaxNumeric},
+        rootIdxTree2
         // {tipSelectedIdx: tipIdx2}
       );
       dispatchObj.tangleTipLookup = constructVisibleTipLookupBetweenTrees(tree.nodes, treeToo.nodes, data.visibility, dataToo.visibility);
@@ -121,7 +123,7 @@ export const changeDateFilter = ({newMin = false, newMax = false, quickdraw = fa
       dateMinNumeric: newMin ? calendarToNumeric(newMin) : controls.dateMinNumeric,
       dateMaxNumeric: newMax ? calendarToNumeric(newMax) : controls.dateMaxNumeric
     };
-    const data = calculateVisiblityAndBranchThickness(tree, controls, dates);
+    const data = calculateVisiblityAndBranchThickness(tree, controls, dates, tree.idxOfInViewRootNode);
     const dispatchObj = {
       type: types.CHANGE_DATES_VISIBILITY_THICKNESS,
       quickdraw,
@@ -137,7 +139,7 @@ export const changeDateFilter = ({newMin = false, newMax = false, quickdraw = fa
       stateCountAttrs: Object.keys(controls.filters)
     };
     if (controls.showTreeToo) {
-      const dataToo = calculateVisiblityAndBranchThickness(treeToo, controls, dates);
+      const dataToo = calculateVisiblityAndBranchThickness(treeToo, controls, dates, treeToo.idxOfInViewRootNode);
       dispatchObj.tangleTipLookup = constructVisibleTipLookupBetweenTrees(tree.nodes, treeToo.nodes, data.visibility, dataToo.visibility);
       dispatchObj.visibilityToo = dataToo.visibility;
       dispatchObj.visibilityVersionToo = dataToo.visibilityVersion;
diff --git a/src/util/treeVisibilityHelpers.js b/src/util/treeVisibilityHelpers.js
index 6340982b..431b3319 100644
--- a/src/util/treeVisibilityHelpers.js
+++ b/src/util/treeVisibilityHelpers.js
@@ -133,7 +133,7 @@ FILTERS:
  - filters (in this code) is a list of filters to apply
    e.g. [{trait: "country", values: [...]}, ...]
 */
-export const calcVisibility = (tree, controls, dates) => {
+export const calcVisibility = (tree, controls, dates, idxOfInViewRootNode) => {
   if (tree.nodes) {
     /* inView represents nodes that are within the current view window (i.e. not off the screen) */
     let inView;
@@ -170,8 +170,21 @@ export const calcVisibility = (tree, controls, dates) => {
         makeParentVisible(filtered, tree.nodes[idxsOfFilteredTips[i]]);
       }
       /* Recursivley hide ancestor nodes that are not the last common
-       * ancestor of selected nodes, starting from the root of the tree */
-      hideNodeIfOnlyOneChildVisible(filtered, tree.nodes[0]);
+        * ancestor of selected nodes, starting from the root of the tree.
+        * This is done iff there are no tips which match the filters outside of the current viewport.
+        */
+      const treeIsZoomed = idxOfInViewRootNode !== 0;
+      let visibleNodeOutsideOfViewport = true;
+      for (const [idx, d] of tree.nodes.entries()) {
+        if (!d.hasChildren && !inView[idx] && filters.every((f) => f.values.includes(getTraitFromNode(d, f.trait)))) {
+          console.log("Exists a tip outside the current viewport which is part of the filtering selection");
+          visibleNodeOutsideOfViewport = true;
+          break;
+        }
+      }
+      if (!(treeIsZoomed && visibleNodeOutsideOfViewport)) {
+        hideNodeIfOnlyOneChildVisible(filtered, tree.nodes[0]);
+      }
     }
     /* intersect the various arrays contributing to visibility */
     const visibility = tree.nodes.map((node, idx) => {
@@ -202,8 +215,8 @@ export const calcVisibility = (tree, controls, dates) => {
   return NODE_VISIBLE;
 };
 
-export const calculateVisiblityAndBranchThickness = (tree, controls, dates) => {
-  const visibility = calcVisibility(tree, controls, dates);
+export const calculateVisiblityAndBranchThickness = (tree, controls, dates, idxOfInViewRootNode) => {
+  const visibility = calcVisibility(tree, controls, dates, idxOfInViewRootNode);
   /* recalculate tipCounts over the tree - modifies redux tree nodes in place (yeah, I know) */
   calcTipCounts(tree.nodes[0], visibility);
   /* re-calculate branchThickness (inline) */

[jameshadfield]

Can we rename this hideNodesAboveCommonVisibleAncestor or similar? This function allows nodes with only one visible child if they are below (closer to the tips than) the CA node.

[joverlee521]

Changed the function name in the latest commit

[jameshadfield]

(Beyond the scope of this PR, but this may be a good place to set some property / state defining the CA of the filtered tips, which will be needed for #1132, although dates will also have to be taken into account for that feature.)

[joverlee521]

Yeah, I can try to tackle #1132 next in a separate PR.

[trvrb]

That would be awesome. #1132 I think would add a whole bunch to usability.

[trvrb]

Awesome work with this @joverlee521! I think the resulting UI is little finicky when we have situations like: https://auspice-filter-branch-t-nxoihy.herokuapp.com/zika?f_country=Angola, where it becomes more difficult to zoom into the clade of interest. With the live behavior you effectively can click multiple times on the parental lineage to get this zoom accomplished. That said, I think the right UI to aim for is to combine this PR with #1132 (zoom to selected button). This should make it easy to filter down and see what you're getting and then zoom-to-selected to fill the viewport.

@jameshadfield: Thanks for thinking this through. This is a tough one, but I think I actually prefer B above. I'd like to keep a firm semantic concept on what we considered as "filtered". Currently, if we zoom to a clade we consider to have filtered to this clade and remove other tips from the map etc... If we've filtered to say just USVI (https://auspice-filter-branch-t-nxoihy.herokuapp.com/zika?f_country=Usvi) and then zoom to the upper clade, I'd consider only these 19 tips to be selected. I would apply consistent logic and then just highlight the TMRCA of these tips. If you leave the thick ancestral branch, I wouldn't be sure if the other tips were selected, but just "off screen" and I wouldn't know what to expect if I click a "zoom to selected" button. Or if I downloaded the tree I'd expect to get the off-screen branches as well.

Also, I know it would be quite a can of worms, but fixing the t-bar issue would generally make these filtered trees much more attractive. But this is a very separate issue.

[jameshadfield]

Thanks @trvrb -- Agree that #1132 becomes even higher priority with the improvements from this PR. (And I've focused on those T-bars for nearly 3 years now! #508)

@joverlee521 -- given trevor's comments above the algorithm you've implemented is 💯 and all that's needed is a slight function rename.

[rneher]

looks good to me. In your solution B, would it make sense to include the branch leading to the MRCA of the selection?

[joverlee521]

Closing in favor of #1257