From 5efc8a59f582e561c3a5dbf4f487d9c1d0f251e7 Mon Sep 17 00:00:00 2001 From: Trevor Bedford Date: Tue, 26 Jan 2021 21:41:51 -0800 Subject: [PATCH 1/4] Enforce unnormalized frequencies when data is lacking This commit forces controls.normalizeFrequencies to be false if there are any pivots where the total frequency is less than 0.1%. This addresses two issues: 1. In the existing code, attempting to normalize situations where pivots have 0% total frequency results in bad looking "all equal" bands. This commit removes the capacity to get into this bad looking app state. 2. When filtering to a particular clade, we often want to switch to unnormalized frequencies anyway (as opposed to filtering to geography). This commit accomplishes this automatically because filtering to an emerging clade will generally result in pivots with <0.1% frequency. --- src/actions/frequencies.js | 14 ++++++++- src/actions/recomputeReduxState.js | 14 ++++++++- .../controls/frequency-normalization.js | 16 ++++++++-- src/util/processFrequencies.js | 29 +++++++++++++++++++ 4 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/actions/frequencies.js b/src/actions/frequencies.js index b49b2bf86..a48e61fed 100644 --- a/src/actions/frequencies.js +++ b/src/actions/frequencies.js @@ -1,7 +1,7 @@ import { debounce } from 'lodash'; import * as types from "./types"; import { timerStart, timerEnd } from "../util/perf"; -import { computeMatrixFromRawData, processFrequenciesJSON } from "../util/processFrequencies"; +import { computeMatrixFromRawData, checkIfNormalizableFromRawData, processFrequenciesJSON } from "../util/processFrequencies"; export const loadFrequencies = (json) => (dispatch, getState) => { const { tree, controls } = getState(); @@ -22,6 +22,18 @@ const updateFrequencyData = (dispatch, getState) => { console.error("Race condition in updateFrequencyData. Frequencies data not in state. Matrix can't be calculated."); return; } + + const allowNormalization = checkIfNormalizableFromRawData( + frequencies.data, + frequencies.pivots, + tree.nodes, + tree.visibility + ); + + if (!allowNormalization) { + controls.normalizeFrequencies = false; + } + const matrix = computeMatrixFromRawData( frequencies.data, frequencies.pivots, diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 4eff90912..edb2aefae 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -12,7 +12,7 @@ import { treeJsonToState } from "../util/treeJsonProcessing"; import { entropyCreateState } from "../util/entropyCreateStateFromJsons"; import { determineColorByGenotypeMutType, calcNodeColor } from "../util/colorHelpers"; import { calcColorScale, createVisibleLegendValues } from "../util/colorScale"; -import { computeMatrixFromRawData } from "../util/processFrequencies"; +import { computeMatrixFromRawData, checkIfNormalizableFromRawData } from "../util/processFrequencies"; import { applyInViewNodesToTree } from "../actions/tree"; import { isColorByGenotype, decodeColorByGenotype, decodeGenotypeFilters, encodeGenotypeFilters } from "../util/getGenotype"; import { getTraitFromNode, getDivFromNode, collectGenotypeStates } from "../util/treeMiscHelpers"; @@ -829,6 +829,18 @@ export const createStateFromQueryOrJSONs = ({ /* update frequencies if they exist (not done for new JSONs) */ if (frequencies && frequencies.loaded) { frequencies.version++; + + const allowNormalization = checkIfNormalizableFromRawData( + frequencies.data, + frequencies.pivots, + tree.nodes, + tree.visibility + ); + + if (!allowNormalization) { + controls.normalizeFrequencies = false; + } + frequencies.matrix = computeMatrixFromRawData( frequencies.data, frequencies.pivots, diff --git a/src/components/controls/frequency-normalization.js b/src/components/controls/frequency-normalization.js index bdaa22193..9b1894857 100644 --- a/src/components/controls/frequency-normalization.js +++ b/src/components/controls/frequency-normalization.js @@ -5,7 +5,7 @@ import { withTranslation } from "react-i18next"; import Toggle from "./toggle"; import { controlsWidth } from "../../util/globals"; import { FREQUENCY_MATRIX } from "../../actions/types"; -import { computeMatrixFromRawData } from "../../util/processFrequencies"; +import { computeMatrixFromRawData, checkIfNormalizableFromRawData } from "../../util/processFrequencies"; @connect((state) => { return { @@ -23,7 +23,19 @@ class NormalizeFrequencies extends React.Component { display on={this.props.controls.normalizeFrequencies} callback={() => { - const normalizeFrequencies = !this.props.controls.normalizeFrequencies; + let normalizeFrequencies = !this.props.controls.normalizeFrequencies; + + const allowNormalization = checkIfNormalizableFromRawData( + this.props.frequencies.data, + this.props.frequencies.pivots, + this.props.tree.nodes, + this.props.tree.visibility + ); + + if (!allowNormalization) { + normalizeFrequencies = false; + } + const matrix = computeMatrixFromRawData( this.props.frequencies.data, this.props.frequencies.pivots, diff --git a/src/util/processFrequencies.js b/src/util/processFrequencies.js index 3df05e8d1..7e6fcd144 100644 --- a/src/util/processFrequencies.js +++ b/src/util/processFrequencies.js @@ -34,6 +34,23 @@ const assignCategory = (colorScale, categories, node, colorBy, isGenotype) => { return unassigned_label; }; +// Returns a boolean specifying if frequencies are allowed to be normalized +// Only normalize if minimum frequency is above 0.1% +export const checkIfNormalizableFromRawData = (data, pivots, nodes, visibility) => { + const pivotsLen = pivots.length; + const pivotTotals = new Array(pivotsLen).fill(0); + data.forEach((d) => { + if (visibility[d.idx] === NODE_VISIBLE) { + for (let i = 0; i < pivotsLen; i++) { + pivotTotals[i] += d.values[i]; + } + } + }); + const minFrequency = Math.min(...pivotTotals); + const allowNormalization = minFrequency > 0.001; + return allowNormalization; +}; + export const computeMatrixFromRawData = (data, pivots, nodes, visibility, colorScale, colorBy, normalizeFrequencies) => { /* color scale domain forms the categories in the stream graph */ const categories = colorScale.legendValues.filter((d) => d !== undefined); @@ -98,6 +115,18 @@ export const processFrequenciesJSON = (rawJSON, tree, controls) => { weight: rawJSON[n.name].weight }); }); + + const allowNormalization = checkIfNormalizableFromRawData( + data, + pivots, + tree.nodes, + tree.visibility + ); + + if (!allowNormalization) { + controls.normalizeFrequencies = false; + } + const matrix = computeMatrixFromRawData( data, pivots, From bbef8f80af0309413c000055eba54411fb42c72d Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 9 Mar 2021 15:32:04 +1300 Subject: [PATCH 2/4] Indicate if frequencies can be normalized in sidebar Previous implementation continued to display the toggle icon but disabled its functionality (by forcing `normalizeFrequencies` to be `false`). Here we replace the toggle with a "not available" message & update the info-popup text. --- .../controls/frequency-normalization.js | 28 +++++++++++-------- src/components/controls/miscInfoText.js | 1 + 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/components/controls/frequency-normalization.js b/src/components/controls/frequency-normalization.js index 9b1894857..e9bcbfb54 100644 --- a/src/components/controls/frequency-normalization.js +++ b/src/components/controls/frequency-normalization.js @@ -6,6 +6,7 @@ import Toggle from "./toggle"; import { controlsWidth } from "../../util/globals"; import { FREQUENCY_MATRIX } from "../../actions/types"; import { computeMatrixFromRawData, checkIfNormalizableFromRawData } from "../../util/processFrequencies"; +import { SidebarSubtitle } from "./styles"; @connect((state) => { return { @@ -17,24 +18,27 @@ import { computeMatrixFromRawData, checkIfNormalizableFromRawData } from "../../ class NormalizeFrequencies extends React.Component { render() { const { t } = this.props; + + const allowNormalization = this.props.frequencies.loaded && this.props.tree.loaded && + checkIfNormalizableFromRawData( + this.props.frequencies.data, + this.props.frequencies.pivots, + this.props.tree.nodes, + this.props.tree.visibility + ); + if (!allowNormalization) { + return ( + (Frequencies cannot be normalized) + ); + } + return (
{ - let normalizeFrequencies = !this.props.controls.normalizeFrequencies; - - const allowNormalization = checkIfNormalizableFromRawData( - this.props.frequencies.data, - this.props.frequencies.pivots, - this.props.tree.nodes, - this.props.tree.visibility - ); - - if (!allowNormalization) { - normalizeFrequencies = false; - } + const normalizeFrequencies = !this.props.controls.normalizeFrequencies; const matrix = computeMatrixFromRawData( this.props.frequencies.data, diff --git a/src/components/controls/miscInfoText.js b/src/components/controls/miscInfoText.js index 517199042..2ed4bac57 100644 --- a/src/components/controls/miscInfoText.js +++ b/src/components/controls/miscInfoText.js @@ -38,5 +38,6 @@ export const PanelOptionsInfo = ( export const FrequencyInfo = ( <> Normalize frequencies controls whether the vertical axis represents the entire dataset or only the samples currently visible (e.g. due to filtering). + This option is not available when data is limited to prevent numerical issues. ); From 0de2197439c13989eb8bdfcbd407d165a7fe6d89 Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 9 Mar 2021 15:51:52 +1300 Subject: [PATCH 3/4] Update normalizeFrequencies flag via redux actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upon initial parsing of the frequencies JSON as well as frequency data updates we may set redux→controls→normalizeFrequencies→false. This commit modifies the LOAD_FREQUENCIES and FREQUENCY_MATRIX actions to pass this information to the reducer, rather than updating the redux state directly from within the actions. I couldn't find any bugs caused by the previous implementation, but this change is more in line with suggested behaviour and should help future-proof work here. --- src/actions/frequencies.js | 20 +++++++------------- src/reducers/controls.js | 2 ++ src/util/processFrequencies.js | 17 +++++------------ 3 files changed, 14 insertions(+), 25 deletions(-) diff --git a/src/actions/frequencies.js b/src/actions/frequencies.js index a48e61fed..8987bc1fb 100644 --- a/src/actions/frequencies.js +++ b/src/actions/frequencies.js @@ -5,9 +5,11 @@ import { computeMatrixFromRawData, checkIfNormalizableFromRawData, processFreque export const loadFrequencies = (json) => (dispatch, getState) => { const { tree, controls } = getState(); + const { data, pivots, matrix, projection_pivot, normalizeFrequencies } = processFrequenciesJSON(json, tree, controls); dispatch({ type: types.LOAD_FREQUENCIES, - frequencies: {loaded: true, version: 1, ...processFrequenciesJSON(json, tree, controls)} + frequencies: {loaded: true, version: 1, data, pivots, matrix, projection_pivot}, + normalizeFrequencies }); }; @@ -23,16 +25,8 @@ const updateFrequencyData = (dispatch, getState) => { return; } - const allowNormalization = checkIfNormalizableFromRawData( - frequencies.data, - frequencies.pivots, - tree.nodes, - tree.visibility - ); - - if (!allowNormalization) { - controls.normalizeFrequencies = false; - } + const normalizeFrequencies = controls.normalizeFrequencies && + checkIfNormalizableFromRawData(frequencies.data, frequencies.pivots, tree.nodes, tree.visibility); const matrix = computeMatrixFromRawData( frequencies.data, @@ -41,10 +35,10 @@ const updateFrequencyData = (dispatch, getState) => { tree.visibility, controls.colorScale, controls.colorBy, - controls.normalizeFrequencies + normalizeFrequencies ); timerEnd("updateFrequencyData"); - dispatch({type: types.FREQUENCY_MATRIX, matrix}); + dispatch({type: types.FREQUENCY_MATRIX, matrix, normalizeFrequencies}); }; /* debounce works better than throttle, as it _won't_ update while events are still coming in (e.g. dragging the date slider) */ diff --git a/src/reducers/controls.js b/src/reducers/controls.js index e5942e415..e027578bc 100644 --- a/src/reducers/controls.js +++ b/src/reducers/controls.js @@ -285,6 +285,8 @@ const Controls = (state = getDefaultControlsState(), action) => { case types.TOGGLE_TRANSMISSION_LINES: return Object.assign({}, state, { showTransmissionLines: action.data }); + case types.LOAD_FREQUENCIES: + return {...state, normalizeFrequencies: action.normalizeFrequencies}; case types.FREQUENCY_MATRIX: { if (Object.hasOwnProperty.call(action, "normalizeFrequencies")) { return Object.assign({}, state, { normalizeFrequencies: action.normalizeFrequencies }); diff --git a/src/util/processFrequencies.js b/src/util/processFrequencies.js index 7e6fcd144..8a06cfc36 100644 --- a/src/util/processFrequencies.js +++ b/src/util/processFrequencies.js @@ -116,16 +116,8 @@ export const processFrequenciesJSON = (rawJSON, tree, controls) => { }); }); - const allowNormalization = checkIfNormalizableFromRawData( - data, - pivots, - tree.nodes, - tree.visibility - ); - - if (!allowNormalization) { - controls.normalizeFrequencies = false; - } + const normalizeFrequencies = controls.normalizeFrequencies && + checkIfNormalizableFromRawData(data, pivots, tree.nodes, tree.visibility); const matrix = computeMatrixFromRawData( data, @@ -134,12 +126,13 @@ export const processFrequenciesJSON = (rawJSON, tree, controls) => { tree.visibility, controls.colorScale, controls.colorBy, - controls.normalizeFrequencies + normalizeFrequencies ); return { data, pivots, matrix, - projection_pivot + projection_pivot, + normalizeFrequencies }; }; From 658f8dccc34c1789befe1c13589980bd13a099e1 Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 9 Mar 2021 16:42:45 +1300 Subject: [PATCH 4/4] Update frequencies panel when data changes The frequencies component performed some basic comparisons between the previously rendered data and new data to avoid unnecessary re-renders. This logic was too simplistic and caused a bug where the component wouldn't re-render the graph when the data had indeed changed. This commit skips these checks. This may result in some unnecessary re-renders, however I couldn't find any in my testing. Closes #1224 --- src/components/frequencies/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/frequencies/index.js b/src/components/frequencies/index.js index b6001fc18..3aa4833fe 100644 --- a/src/components/frequencies/index.js +++ b/src/components/frequencies/index.js @@ -5,7 +5,7 @@ import 'd3-transition'; import { connect } from "react-redux"; import Card from "../framework/card"; import { calcXScale, calcYScale, drawXAxis, drawYAxis, drawProjectionInfo, - areListsEqual, drawStream, processMatrix, parseColorBy, normString } from "./functions"; + drawStream, processMatrix, parseColorBy, normString } from "./functions"; import "../../css/entropy.css"; @connect((state) => { @@ -50,8 +50,6 @@ class Frequencies extends React.Component { /* we don't have to check width / height changes here - that's done in componentDidUpdate */ const data = processMatrix({...newProps}); const maxYChange = oldState.maxY !== data.maxY; - const catChange = !areListsEqual(oldState.categories, data.categories); - if (!maxYChange && !catChange) return false; const chartGeom = this.calcChartGeom(newProps.width, newProps.height); /* should the y scale be updated? */ let newScales;