From c0baa808455f9f5b22283e6f7d346773a0ead96c Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 14 Mar 2023 16:01:48 +1300 Subject: [PATCH] Fix bounds matching for zero values This fixes a common bug for continuous scales where nodes have (valid) zero values. The bound for the first (smallest) legend entry would have zero as it's lower value, and as we strictly matched `v \in [a,b)` the node was never included as a match. This presented in two places: 1. Hovering over a legend swatch would not highlight (increase the radius of) matching nodes whose value was zero 2. The frequencies stream graph would map zeros to "unassigned" rather than their correct legend bin. There were a few code changes needed to case (2) as we also had falsey checks (and 0 is falsey). Note that the algorithm to create legend bounds is problematic and should be rewritten. This bug was first identified by Hugh Haddox, but others may have noticed it over the years?!? The /flu/seasonal/h3n2/ha/2y dataset is useful to test this bug: * `c=ne_star` - most of the tree has zeros, none are unassigned * `c=projected_frequency` - both unassigned and zero tips. * `c=fitness` failure to match lowest legend entry due to the bug in the legend bounds creation --- src/util/processFrequencies.js | 47 +++++++++++++++++++++------------- src/util/tipRadiusHelpers.js | 4 +++ 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/util/processFrequencies.js b/src/util/processFrequencies.js index fafad375b..b103cef21 100644 --- a/src/util/processFrequencies.js +++ b/src/util/processFrequencies.js @@ -5,32 +5,44 @@ import { getTraitFromNode } from "../util/treeMiscHelpers"; export const unassigned_label = "unassigned"; /** - * assign a given node to a category - * Find the colorBy value of the node and, using the colorScale and the provided categories, - * assign the correct category. - * @return {string} category or the unassigned label + * Assign a given node to a category for the current colorScale / colorBy. + * Categories are legend values for the given coloring (or potentially + * a subset of legend values) and therefore will have a corresponding `colorScale.legendBounds`. + * A node value, `v`, is assigned to a category with legend bounds {a,b} if `v \in (a, b]` + * unless we are in the first category in which case it's `v \in [a, b]` (to consider the `v=a` case) + * See also `determineLegendMatch`. + * @return {any} element of `categories` OR `unassigned_label` */ const assignCategory = (colorScale, categories, node, colorBy, isGenotype) => { if (isGenotype) return node.currentGt; const value = getTraitFromNode(node, colorBy); - if (!value || value === "unknown") { - return unassigned_label; + + if (!colorScale.continuous) { + /* Note - the following conditional previously applied to continuous scales + as well, but caused bugs in cases such as `value=0`. We should centralise and clarify + what we mean by unknown values as there may well be bugs in here -- for instance + can ordinal scales have `value=0`? I'm leaving it here for now to preserve previous + behavior for non-continous scales james, march 2023 */ + if (!value || value === "unknown") { + return unassigned_label; + } + return value; } - if (!colorScale.continuous) return value; for (let i = 0; i < categories.length; i++) { - /* roughly the same logic as the determineLegendMatch function */ const category = categories[i]; - if (category === unassigned_label) { - return unassigned_label; - } const lowerBound = colorScale.legendBounds[category][0]; const upperBound = colorScale.legendBounds[category][1]; - if (value <= upperBound && value > lowerBound) { + /* the following looks like it's checking `value \in [a,b]` however because category values + are ascending the case where `value=a` can only occur for the first category (i=0). There + is an assumption here that `b_i = a_{i+1}`; if this is not true then there is a bug in + the generation of the bounds. */ + if (value <= upperBound && value >= lowerBound) { return category; } } - console.error("Could not assign", value, "to a category"); + /* no matching category */ + // console.log("Could not assign", value, "to a category"); return unassigned_label; }; @@ -62,8 +74,8 @@ export const computeMatrixFromRawData = ( ) => { /* color scale domain forms the categories in the stream graph */ const categories = colorScale.legendValues.filter((d) => d !== undefined); - categories.push(unassigned_label); /* for tips without a colorBy */ const isGenotype = isColorByGenotype(colorBy); + /* Matrix shape: first level (keys of Map) are values of the current colorBy. Type is preserved (hence the use of a Map()) second level (elements of array) are the pivots & their frequency value. matrix.get('22L')[3] is the (3+1)th pivot for the color-by value 21L */ @@ -72,15 +84,14 @@ export const computeMatrixFromRawData = ( categories.forEach((x) => { matrix.set(x, new Array(pivotsLen).fill(0)); }); + matrix.set(unassigned_label, new Array(pivotsLen).fill(0)); // Will be removed later if no nodes assigned. + // let debugTipsSeen = 0; const debugPivotTotals = new Array(pivotsLen).fill(0); data.forEach((d) => { if (visibility[d.idx] === NODE_VISIBLE) { // debugTipsSeen++; - const category = - assignCategory(colorScale, categories, nodes[d.idx], colorBy, isGenotype) || - unassigned_label; - // if (category === unassigned_label) return; + const category = assignCategory(colorScale, categories, nodes[d.idx], colorBy, isGenotype); for (let i = 0; i < pivotsLen; i++) { matrix.get(category)[i] += d.values[i]; debugPivotTotals[i] += d.values[i]; diff --git a/src/util/tipRadiusHelpers.js b/src/util/tipRadiusHelpers.js index ffafa99c1..cab5a06dc 100644 --- a/src/util/tipRadiusHelpers.js +++ b/src/util/tipRadiusHelpers.js @@ -6,6 +6,7 @@ import { getTraitFromNode } from "../util/treeMiscHelpers"; * equates a single tip and a legend element * exact match is required for categorical quantities such as genotypes, regions * continuous variables need to fall into the interval (lower_bound, upper_bound] +* except for the first (smallest) legend value where we also match value=lower_bound * @param {string|number} selectedLegendItem e.g. "USA" or 2021 * @param {object} node - node (tip) in question * @param {object} colorScale - used to get the value of the attribute being used for colouring @@ -14,6 +15,9 @@ import { getTraitFromNode } from "../util/treeMiscHelpers"; export const determineLegendMatch = (selectedLegendItem, node, colorScale) => { const nodeAttr = getTipColorAttribute(node, colorScale); if (colorScale.continuous) { + if (selectedLegendItem === colorScale.legendValues[0] && nodeAttr===colorScale.legendBounds[selectedLegendItem][0]) { + return true; + } return (nodeAttr <= colorScale.legendBounds[selectedLegendItem][1]) && (nodeAttr > colorScale.legendBounds[selectedLegendItem][0]); }