Fix rounding bug in zoom calculation. #1426
Merged
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
Fixing a rounding error in calculateZoom. Instead of rounding to the closest integer zoom level I believe we should be rounding up to the nearest zoom level which can contain the given window. This fixes a display issue with the ZoomSliderPanel. I'm not 100% sure I understand the potential consequences of changing thhis for other code that relies on it, so it would be good to have a second pair of eyes on it. It seems intuitively to be correct now but it's possible the use case of "zoom level which is closest to the width of the given window" was really what was wanted. I haven't found any obvious issues with some manual testing of this change.
jrobinso
pushed a commit
that referenced
this pull request
Dec 4, 2023
Fixing a rounding error in calculateZoom. Instead of rounding to the closest integer zoom level I believe we should be rounding up to the nearest zoom level which can contain the given window. This fixes a display issue with the ZoomSliderPanel. I'm not 100% sure I understand the potential consequences of changing thhis for other code that relies on it, so it would be good to have a second pair of eyes on it. It seems intuitively to be correct now but it's possible the use case of "zoom level which is closest to the width of the given window" was really what was wanted. I haven't found any obvious issues with some manual testing of this change.
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.
Fixing a rounding error in calculateZoom. Instead of rounding to the closest integer zoom level I believe we should be rounding up to the nearest zoom level which can contain the given window.
This fixes a display issue with the ZoomSliderPanel. Certain chromosomes would display the blue mark off by 1 because it was sometimes rounding up and sometimes down.
I'm not 100% sure I understand the potential consequences of changing this for other code that relies on it, so it would be good to have a second pair of eyes on it. It seems intuitively to be correct now but it's possible the use case of "zoom level which is closest to the width of the given window" was really what was wanted. I haven't found any obvious issues with some manual testing of this change.