Enforce unnormalized frequencies when data is lacking #1278
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
jameshadfield
temporarily deployed
to
auspice-frequencies-def-mldq3u
|
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
|
jameshadfield
approved these changes
Mar 9, 2021
|
Fantastic! Thanks for the additional cleanup here @jameshadfield. And thanks for the redux help. This is ready to be merged from my perspective. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
This commit forces
controls.normalizeFrequenciesto 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 = falsethat 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.