Skip to content

Commit

Permalink
Fix bounds matching for zero values
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jameshadfield committed Mar 14, 2023
1 parent 57c27b3 commit c0baa80
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 18 deletions.
47 changes: 29 additions & 18 deletions src/util/processFrequencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down Expand Up @@ -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 */
Expand All @@ -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];
Expand Down
4 changes: 4 additions & 0 deletions src/util/tipRadiusHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]);
}
Expand Down

0 comments on commit c0baa80

Please sign in to comment.