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

Improve tree performance for trees with over 4000 tips #1880

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/components/tree/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ const Tree = connect((state: RootState) => ({
tipLabelKey: state.controls.tipLabelKey,
narrativeMode: state.narrative.display,
animationPlayPauseButton: state.controls.animationPlayPauseButton,
showOnlyPanels: state.controls.showOnlyPanels
showOnlyPanels: state.controls.showOnlyPanels,
performanceToggles: state.controls.performanceToggles,
}))(UnconnectedTree);

export default Tree;
19 changes: 9 additions & 10 deletions src/components/tree/phyloTree/change.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,12 @@ const createUpdateCall = (treeElem, properties) => (selection) => {

const genericSelectAndModify = (svg, treeElem, updateCall, transitionTime) => {
// console.log("general svg update for", treeElem);
svg.selectAll(treeElem)
.filter((d) => d.update)
.transition().duration(transitionTime)
.call(updateCall);
if (!transitionTime) {
/* https://github.com/d3/d3-timer#timerFlush */
timerFlush();
// console.log("\t\t--FLUSHING TIMER--");
let selection = svg.selectAll(treeElem)
.filter((d) => d.update);
if (transitionTime) {
selection = selection.transition().duration(transitionTime);
}
selection.call(updateCall);
};

/* use D3 to select and modify elements, such that a given element is only ever modified _once_
Expand Down Expand Up @@ -271,7 +268,8 @@ export const change = function change({
branchThickness = undefined,
/* other data */
focus = undefined,
scatterVariables = undefined
scatterVariables = undefined,
performanceToggles = {},
}) {
// console.log("\n** phylotree.change() (time since last run:", Date.now() - this.timeLastRenderRequested, "ms) **\n\n");
timerStart("phylotree.change()");
Expand All @@ -280,10 +278,11 @@ export const change = function change({
const svgPropsToUpdate = new Set(); /* which SVG properties shall be changed. E.g. "fill", "stroke" */
const useModifySVGInStages = newLayout; /* use modifySVGInStages rather than modifySVG. Not used often. */


/* calculate dt */
const idealTransitionTime = 500;
let transitionTime = idealTransitionTime;
if ((Date.now() - this.timeLastRenderRequested) < idealTransitionTime * 2) {
if ((Date.now() - this.timeLastRenderRequested) < idealTransitionTime * 2 || performanceToggles.get("skipTreeAnimation")===true) {
transitionTime = 0;
}

Expand Down
11 changes: 5 additions & 6 deletions src/components/tree/phyloTree/labels.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { timerFlush } from "d3-timer";
import { NODE_VISIBLE } from "../../../util/globals";
import { numericToDateObject, prettifyDate } from "../../../util/dateHelpers";
import { getTraitFromNode } from "../../../util/treeMiscHelpers";
Expand Down Expand Up @@ -108,16 +107,16 @@ export const updateBranchLabels = function updateBranchLabels(dt) {
);
const labelSize = branchLabelSize(this.params.branchLabelKey);
const fontWeight = branchLabelFontWeight(this.params.branchLabelKey);
this.groups.branchLabels
let selection = this.groups.branchLabels
.selectAll(".branchLabel")
.transition()
.duration(dt)
.attr("x", (d) => d.xTip - 5)
if (dt) {
selection = selection.transition().duration(dt);
}
selection.attr("x", (d) => d.xTip - 5)
.attr("y", (d) => d.yTip - this.params.branchLabelPadY)
.style("visibility", visibility)
.style("font-weight", fontWeight)
.style("font-size", labelSize);
if (!dt) timerFlush();
};

export const removeBranchLabels = function removeBranchLabels() {
Expand Down
1 change: 1 addition & 0 deletions src/components/tree/reactD3Interface/change.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export const changePhyloTreeViaPropsComparison = (mainTree, phylotree, oldProps,
const change = Object.keys(args).length;
if (change) {
args.animationInProgress = newProps.animationPlayPauseButton === "Pause";
args.performanceToggles = newProps.performanceToggles;
// console.log('\n\n** ', phylotree.id, 'changePhyloTreeViaPropsComparison **', args);
phylotree.change(args);
}
Expand Down
28 changes: 28 additions & 0 deletions src/middleware/performanceToggles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import * as types from "../actions/types";

/**
* Performance toggles (reduxState.controls.performanceToggles) represent flags
* for which we enable/disable certain functionality in Auspice. These flags
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Toggle" in the name confused me at first since all other usages of "toggle" refer to booleans in Redux that are flipped on dispatch. This is instead flipped by preset conditions. How about "performance optimizations"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will change to performanceFlags

* shouldn't be depended on, i.e. Auspice should work just fine without them
* (but may be a little slow).
*/


export const performanceToggles = (_store) => (next) => (action) => {
let modifiedAction;
switch (action.type) {
case types.URL_QUERY_CHANGE_WITH_COMPUTED_STATE: /* fallthrough */
case types.CLEAN_START: {
victorlin marked this conversation as resolved.
Show resolved Hide resolved
modifiedAction = {...action};
modifiedAction.controls.performanceToggles = calculate(action)
}
}
return next(modifiedAction || action); // send action to other middleware / reducers
};

function calculate({tree}) {
const toggles = new Map();
const totalTipCount = tree?.nodes?.[0]?.fullTipCount;
toggles.set("skipTreeAnimation", totalTipCount > 4000);
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: 0862223

For future, we should add nuance to this performance flag. For instance,
viewing a 2k tree and zooming into a clade has poor performance and we'd
be better off skipping the animation, however once in a small clade it
works well. For that same tree (viewing all 2k tips) filtering to a
clade still looks good with the animation.

I thought about something like tree.nodes.[tree.idxOfInViewRootNode].fullTipCount > 2000 but that would only work for half the time, e.g. from 4000 → 100 or 100 → 4000 but not both. This would have to be set somewhere like modifyTreeStateVisAndBranchThickness where both the old and new idxOfInViewRootNode are accessible and make the condition something like tree.nodes.[oldState.idxOfInViewRootNode].fullTipCount > 2000 || tree.nodes.[newState.idxOfInViewRootNode].fullTipCount > 2000.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In canonical redux this would be straightforward as middleware has access to the old state and because we use fat actions it also can see the new state. But as we modify the nodes directly (i.e. they're not immutable, largely for performance reasons) we can't leverage this. But we can see old and new idxOfInViewRootNode so we could implement the last bit of code you wrote within the middleware. (Would require quite a bit of testing!)

return toggles;
}
3 changes: 2 additions & 1 deletion src/reducers/controls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ export const getDefaultControlsState = () => {
measurementsDisplay: undefined,
measurementsShowOverallMean: undefined,
measurementsShowThreshold: undefined,
measurementsFilters: {}
measurementsFilters: {},
performanceToggles: new Map(),
};
};

Expand Down
2 changes: 2 additions & 0 deletions src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { changeURLMiddleware } from "./middleware/changeURL";
import rootReducer from "./reducers";
// import { loggingMiddleware } from "./middleware/logActions";
import { keepScatterplotStateInSync } from "./middleware/scatterplot";
import { performanceToggles } from "./middleware/performanceToggles";

const middleware = [
keepScatterplotStateInSync,
changeURLMiddleware,
performanceToggles,
// loggingMiddleware
];

Expand Down