Entropy bug fixes #1906
Merged
Entropy bug fixes #1906
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
Changing between "Entropy - Events" results in two dispatches (`ENTROPY_COUNTS_TOGGLE` and `ENTROPY_DATA`) however the entropy code would ignore the first update if it didn't contain new entropy data. This bug has probably existed for the life of the entropy panel however was masked by those two actions happening back-to-back and being (always?) bundled together by redux. Recent work in #1879 (v2.60.0) meant these actions were no longer back-to-back and thus the bug was revealed. Closes #1905
This reverts commit 0c91724. That commit was the first performance optimisation within #1879. The idea was to skip the expensive recalculation when initially dragging the date slider. In other contexts this change was a negative, specifically when there was only one action (e.g. changing entropy to events) as we essentially introduced a 500ms delay. Note that when the panel is off-screen subsequent work in #1879 means that the recalculation is skipped anyway, so reverting this commit only affects performance when the panel is on screen and we're dragging the date slider. To remedy this we should add a code path which specifically skips the leading edge calculation when dragging the date slider. Essentially closes #1899 - that issue was partly the recalculation on-demand which remains, but greatly exacerbated by the unnecessary 500ms delay.
nextstrain-bot
temporarily deployed
to
auspice-james-entropy-f-ztd4ke
genehack
approved these changes
Nov 18, 2024
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.
commit 1
Changing between "Entropy - Events" results in two dispatches
(
ENTROPY_COUNTS_TOGGLEandENTROPY_DATA) however the entropy codewould ignore the first update if it didn't contain new entropy data.
This bug has probably existed for the life of the entropy panel however
was masked by those two actions happening back-to-back and being
(always?) bundled together by redux.
Recent work in #1879 (v2.60.0) meant these actions were no longer
back-to-back and thus the bug was revealed.
Closes #1905
commit 2
This reverts commit 0c91724.
That commit was the first performance optimisation within #1879. The
idea was to skip the expensive recalculation when initially dragging the
date slider. In other contexts this change was a negative, specifically
when there was only one action (e.g. changing entropy to events) as we
essentially introduced a 500ms delay.
Note that when the panel is off-screen subsequent work in #1879 means
that the recalculation is skipped anyway, so reverting this commit only
affects performance when the panel is on screen and we're dragging the
date slider. To remedy this we should add a code path which specifically
skips the leading edge calculation when dragging the date slider.
Essentially closes #1899 - that issue was partly the recalculation
on-demand which remains, but greatly exacerbated by the unnecessary
500ms delay.