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

Make frequencies tend to 0 in absence of data #1325

Merged
merged 6 commits into from
Apr 10, 2021
Merged

Conversation

rneher
Copy link
Member

@rneher rneher commented Mar 30, 2021

No description provided.

@jameshadfield jameshadfield temporarily deployed to auspice-explore-normali-uyjuwf March 30, 2021 21:43 Inactive
@rneher rneher requested review from trvrb and jameshadfield April 6, 2021 17:46
@rneher
Copy link
Member Author

rneher commented Apr 6, 2021

@jameshadfield, can one redeploy this? I increased the padding value for the frequencies a bit which should improve things. But would be good to flip through various builds and play with it a bit.

@trvrb
Copy link
Member

trvrb commented Apr 6, 2021

@rneher: You should be able to do this through Heroku. Go here https://dashboard.heroku.com/teams/nextstrain/apps, click on "Auspice" and then find the branch you want to deploy and click "Create Review App". I've just done this for explore-normalization-fix.

@trvrb trvrb temporarily deployed to auspice-explore-normali-hknihq April 6, 2021 19:47 Inactive
@rneher
Copy link
Member Author

rneher commented Apr 6, 2021

Thanks, i keep forgetting how this works for auspice. With the increased padding parameter, the transition to 0 happens a little earlier and a little less abrupt:
Screenshot from 2021-04-06 21-55-52

@rneher
Copy link
Member Author

rneher commented Apr 6, 2021

I do think this is an improvement compared to disallowing normalization. But an explicit grey out would still be better.

@rneher
Copy link
Member Author

rneher commented Apr 6, 2021

By changing minVal

https://github.com/nextstrain/auspice/pull/1325/files#diff-522f919569fe474196547277ab2bf9a235378aa2b41e86b7982aeb79b323cc70R93

one can move the place where frequencies drop to 0 around and also sharpen the transition (the smaller the value, the earlier and sharper the transition is). This is essentially due to competition between the Gaussian smoothing and the constant minVal. For smaller minVal, the Gaussian dominates for longer.

@trvrb
Copy link
Member

trvrb commented Apr 7, 2021

Thanks Richard. Just clicking around here I noticed: https://auspice-explore-normali-hknihq.herokuapp.com/ncov/global?f_country=Switzerland

Screen Shot 2021-04-06 at 6 07 06 PM

I can't quite tell what's going on with the June 2020 timepoints. And also the slow rise of total frequency in Spring 2020 where there are 7 pivots that don't total 100%.

But maybe the path forward is to do something like this and then place a gray box over pivots that don't have >99% total frequency when normalized? But I would generally prefer a sharper transition. Here we have situations like https://auspice-explore-normali-hknihq.herokuapp.com/ncov/global?c=country&f_country=Vietnam where the whole second peak is less than 97% total frequency. I definitely expected "normalized" frequencies to either sum to 100% or sum to nothing.

@rneher rneher temporarily deployed to auspice-explore-normali-hknihq April 7, 2021 10:20 Inactive
@rneher rneher temporarily deployed to auspice-explore-normali-hknihq April 7, 2021 10:26 Inactive
@rneher
Copy link
Member Author

rneher commented Apr 7, 2021

yeah, this up and down is not good. I changed it to either fully normalized or zero.

@trvrb
Copy link
Member

trvrb commented Apr 7, 2021

Awesome! Much improved. Thanks Richard. I think I'm okay merging this and then doing a separate PR for "gray boxes". Although it looks like there's a test failing due to linting error.

@trvrb trvrb changed the title DRAFT: make frequencies tend to 0 in absense of data Make frequencies tend to 0 in absence of data Apr 7, 2021
@rneher rneher temporarily deployed to auspice-explore-normali-hknihq April 7, 2021 18:31 Inactive
@rneher
Copy link
Member Author

rneher commented Apr 7, 2021

I fixed the linting issue... @jameshadfield, any other comments?

@trvrb
Copy link
Member

trvrb commented Apr 10, 2021

I think this is a definite improvement and would be in support of merging.

@rneher rneher merged commit a0e9080 into master Apr 10, 2021
@rneher rneher deleted the explore-normalization-fix branch April 10, 2021 13:58
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