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

Legend bound creation does not consider observed values #1644

Closed
jameshadfield opened this issue Mar 14, 2023 · 1 comment
Closed

Legend bound creation does not consider observed values #1644

jameshadfield opened this issue Mar 14, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@jameshadfield
Copy link
Member

The creation of legend bounds for continuous scales uses hardcoded minimum (0) and maximum (10,000):

function createLegendBounds(legendValues) {
const valBetween = (x0, x1) => x0 + 0.5*(x1-x0);
const len = legendValues.length;
const legendBounds = {};
legendBounds[legendValues[0]] = [0, valBetween(legendValues[0], legendValues[1])];
for (let i = 1; i < len - 1; i++) {
legendBounds[legendValues[i]] = [valBetween(legendValues[i-1], legendValues[i]), valBetween(legendValues[i], legendValues[i+1])];
}
legendBounds[legendValues[len-1]] = [valBetween(legendValues[len-2], legendValues[len-1]), 10000];
return legendBounds;
}

This is problematic for continuous scales which exceed this range. Using this example, the lowest legend value of -4.51 has legend bounds of [0, -3.97]. This results in problems similar to those fixed in #1643.

A better solution would be to consider all observed values in the tree to create these bounds, especially as we already collect them during continuous scale creation.

The function linked above is used in two other places (ordinal scales which are all integers, fall-through grey-colorscale), and their usage should also be re-evaluated.

@corneliusroemer
Copy link
Member

Is this related to #1626 #1653 ?

@victorlin victorlin moved this from New to Backlog in Nextstrain planning (archived) Mar 22, 2023
jameshadfield added a commit that referenced this issue Mar 27, 2023
Previously the creation of legend bounds for continuous scales used
hardcoded minimum (0) and maximum (10,000). This may result in the
first and/or last legend entry having associated bounds which could
never match any values.

Given the various locations this function is called it's easier to
set the min/max as (-)Infinity rather than consider the observed min/max.

Closes #1644
Closes #1654
jameshadfield added a commit that referenced this issue Mar 28, 2023
Previously the creation of legend bounds for continuous scales used
hardcoded minimum (0) and maximum (10,000). This may result in the
first and/or last legend entry having associated bounds which could
never match any values.

Given the various locations this function is called it's easier to
set the min/max as (-)Infinity rather than consider the observed min/max.

Closes #1644
Closes #1654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

No branches or pull requests

2 participants