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

Enable calculation of unnormalized LBI #380

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Enable calculation of unnormalized LBI #380

merged 1 commit into from
Oct 17, 2019

Conversation

huddlej
Copy link
Contributor

@huddlej huddlej commented Oct 3, 2019

Historically, we have calculated LBI and then normalized all LBI values by the
maximum value in a tree. However, there are cases when users would prefer to
analyze the raw LBI values calculated prior to normalization. For this reason,
this commit modifies the existing LBI function and command line interfaces to
accept an argument that will disable normalization. For backwards compatibility,
normalized LBI output remains the default.

One side effect of this commit is to remove unused kwargs from both the
calculate_LBI and select_nodes_in_season functions.

Historically, we have calculated LBI and then normalized all LBI values by the
maximum value in a tree. However, there are cases when users would prefer to
analyze the raw LBI values calculated prior to normalization. For this reason,
this commit modifies the existing LBI function and command line interfaces to
accept an argument that will disable normalization. For backwards compatibility,
normalized LBI output remains the default.

One side effect of this commit is to remove unused kwargs from both the
`calculate_LBI` and `select_nodes_in_season` functions.
@huddlej huddlej requested a review from rneher October 3, 2019 23:28
@jameshadfield
Copy link
Member

Cool -- quick heads up that the colour scale bounds for lbi of 0 & 0.7 are hardcoded into auspice here https://github.com/nextstrain/auspice/blob/v2/src/util/colorScale.js#L197 so the un-normalised LBI may be confusing if it's still called lbi 😉

One day it'd be great to define desired bounds of continuous variables in the auspice JSONs, but we're not there yet.

@huddlej
Copy link
Contributor Author

huddlej commented Oct 4, 2019

Thanks @jameshadfield ! I just made an issue for this in auspice, so we don't forget. In the meantime, users who want to export unnormalized LBI for visualization in auspice can specify a different attribute name with the required --attribute-names flag.

Since this PR doesn't change the defaults and the special treatment of the lbi attribute is in auspice, I'm ok with not adding any additional documentation here about the limitations of the lbi attribute.

Copy link
Member

@rneher rneher left a comment

Choose a reason for hiding this comment

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

this looks good to me.

@huddlej huddlej merged commit b1507ac into master Oct 17, 2019
@rneher rneher deleted the unnormalized-lbi branch October 27, 2019 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants