add toggle to normalize frequencies #1158
Conversation
rneher
changed the title
rneher
requested review from
jameshadfield and
eharkins
and removed request for
jameshadfield
|
@jameshadfield @eharkins this isn't done yet... the transmission line toggle updates state but doesn't trigger a rerender of the frequencies. Once you update colorBy etc it works as intended. I got stuck on where this needs to be triggered. But the basic behavior is in place. Without this normalization, clade frequencies in Europe look like this: with the toggle true they look like: but to get there I have to change the colorBy... would be great if you could point me to where this goes wrong. |
|
probably easiest to test with a flu data set: |
|
I hooked up the recalculation of frequencies to the toggle. this now seems to work. I also added a note to the title of the frequency panel. |
rneher
changed the title
|
this seem to work for me now. to test, look at a filtered dataset like and toggle the frequency normalizer. |
|
I made a couple of additions in response to suggestions by @huddlej including a check to avoid |
Toggling "normalize frequencies" now results in only a single dispatch call. This reduces the chances of state becoming out-of-sync in future work, and will allow narratives to function better (if we ever use frequencies in narratives). The "TOGGLE_X" actions work best if the react component will update appropriately when that state changes (or some middleware listens for it and updates redux state accordingly). As the frequencies are currently designed, however, the underlying matrix is updated independent of the frequencies panel, and the panel component is more of a plain-renderer.
There was a problem hiding this comment.
Looks great! The dynamic title text is very helpful.
As you've found out, the frequencies behave slightly differently to the tree: For frequencies the underlying matrix is recalculated by actions such as filtering, tree-zooming etc, and the react component is more of a plain renderer of the pre-computed data. So a "toggle" action must recompute the appropriate redux state.
For the map, the react component does a lot of the data recompute itself, and so a "toggle" action would be a very simple action, and the <Map> component would do the appropriate recompute when it noticed that state change.
I added a commit which reduced the number of actions needed (more details in commit message).
Description of proposed changes
Frequencies are currently normalized such that they sum up to 100% when all tips are visible. Sometimes it is useful to normalize them to the currently visible tips (say you filter to Europe an want to see frequencies of clades in Europe -- you'd like to see what fraction 20A has in Europe rather than what fraction European 20A has of all sequences).
This PR implements a toggle switch that allows to flip between these two normalizations.
The implementation is modeled on the showTransmissionLine toogle PR #1147
Related issue(s)
Fixes #784
related to #838
Testing
What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?
Thank you for contributing to Nextstrain!