-
Notifications
You must be signed in to change notification settings - Fork 163
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
Enforce unnormalized frequencies when data is lacking #1278
Conversation
This commit forces controls.normalizeFrequencies to be false if there are any pivots where the total frequency is less than 0.1%. This addresses two issues: 1. In the existing code, attempting to normalize situations where pivots have 0% total frequency results in bad looking "all equal" bands. This commit removes the capacity to get into this bad looking app state. 2. When filtering to a particular clade, we often want to switch to unnormalized frequencies anyway (as opposed to filtering to geography). This commit accomplishes this automatically because filtering to an emerging clade will generally result in pivots with <0.1% frequency.
Hi @jameshadfield I know you have a million priorities right now, if this is a quick review, I'd love some attention here. Filters like https://nextstrain.org/ncov/global?gt=S.501Y or https://nextstrain.org/ncov/global?f_clade_membership=20H/501Y.V2 are super common now, but the resulting rainbow band in the frequencies panel looks pretty broken. I just noticed this when I was interested in frequency of B.1.1.7 across Europe and how this mapped across countries. This was the app state I made for this question: https://nextstrain.org/ncov/europe?dmin=2021-01-01&f_region=Europe showing just samples after Jan 1, but this results in the unfortunate rainbow banding: |
Previous implementation continued to display the toggle icon but disabled its functionality (by forcing `normalizeFrequencies` to be `false`). Here we replace the toggle with a "not available" message & update the info-popup text.
Upon initial parsing of the frequencies JSON as well as frequency data updates we may set redux→controls→normalizeFrequencies→false. This commit modifies the LOAD_FREQUENCIES and FREQUENCY_MATRIX actions to pass this information to the reducer, rather than updating the redux state directly from within the actions. I couldn't find any bugs caused by the previous implementation, but this change is more in line with suggested behaviour and should help future-proof work here.
The frequencies component performed some basic comparisons between the previously rendered data and new data to avoid unnecessary re-renders. This logic was too simplistic and caused a bug where the component wouldn't re-render the graph when the data had indeed changed. This commit skips these checks. This may result in some unnecessary re-renders, however I couldn't find any in my testing. Closes #1224
This looks great! I added a few small commits to
|
Fantastic! Thanks for the additional cleanup here @jameshadfield. And thanks for the redux help. This is ready to be merged from my perspective. |
Description of proposed changes
This commit forces
controls.normalizeFrequencies
to be false if there are any pivots where the total frequency is less than 0.1%.This addresses two issues:
Related issue(s)
Fixes #1225
Testing
Tested locally through
npm run dev
.@jameshadfield --- I'm still not great at redux dataflow and there are places where I set
controls.normalizeFrequencies = false
that may need to be corrected. Everything seemed to work as it should in my testing, but that's not a guarantee that I'm not doing something wrong.