Skip to content
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

Diversity panel extends beyond genome end on 3' end #1651

Closed
corneliusroemer opened this issue Mar 20, 2023 · 2 comments · Fixed by #1684
Closed

Diversity panel extends beyond genome end on 3' end #1651

corneliusroemer opened this issue Mar 20, 2023 · 2 comments · Fixed by #1684
Labels
bug Something isn't working

Comments

@corneliusroemer
Copy link
Member

Current Behavior

The diversity panel x-axis extends beyond the 3' end of the genome.

e.g. for SARS-CoV-2 it goes roughly 150 bases beyond the 3' end at 29903 (Wuhan-Hu-1)
image

This is asymmetric with the 5' end, where it starts at 0 and doesn't go negative:
image

Expected behavior

The position axis does not extend beyond the genome end (at least not more than a few bp)

The fact that the axis extends creates the user impression that the real genome is longer than it actually is, and that there is a lack of mutations - when in fact there can't be any mutations in this non-existent region.

See in our ncov builds: https://next.nextstrain.org/ncov/gisaid/global/1m?gmin=29677&l=unrooted&m=div

@jameshadfield
Copy link
Member

Notes after a quick investigation into this. Relevant provided data (for nCoV):

“nuc”: { “start”: 1, “end”: 29903, ...
“N”: {  “start”: 28274,  “end”: 29533, ...

The lower scale (xNav) correctly sets and maintains a domain of [0, 29903]. The upper scale (xMain) starts with the same domain (assuming no URL query params). All ticks are managed by d3's axisBottom.

There's some space to the right of the lower scale which we can drag the selection box into, which is the cause of this bug. As we drag the selection to the RHS, this function calculates zoom coordinates as high as ~30,036. This has the flow-on effect of changing the domain of the upper scale (here), which is why d3 shows the tick at 30,000bp on the upper axis. The zoom selection can go further than the axis domain due to this code:

this.brush = brushX()
/* the extent is relative to the navGraph group - the constants are a bit hacky... */
.extent([[this.offsets.x1, 0], [this.offsets.width + 20, this.offsets.heightNav - 1 + 25]])

however removing the 20px of padding here introduces another problem (we can no longer drag the selection far enough), presumably the reason for this hack in the first place. I can mostly solve the bug by forcing the zoomCoordinates to never exceed the axis domain, but if we can work out why this hack is needed in extent() that would be the best.

@emmahodcroft
Copy link
Member

I have a vague memory of also coming across this [that without some extra space you can't drag far enough] when I did some work on the diversity bar way back in the day.

jameshadfield added a commit that referenced this issue Aug 2, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 3, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 4, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
@jameshadfield jameshadfield mentioned this issue Aug 4, 2023
17 tasks
jameshadfield added a commit that referenced this issue Aug 7, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 9, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 10, 2023
WIP -- wireframe dots to remove once zooming works

The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 10, 2023
The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
jameshadfield added a commit that referenced this issue Aug 17, 2023
The panel makes extensive use of both SVG transforms on <g> elements as
well as x,y positions of elements inside those groups. The x,y positions
will always be relative to the parent transform¹. All previous
iterations of the entropy panel had gotten quite confused about this,
and I had maintained this confusion in previous commits on this branch.
Our confusion was consistent at least, so there weren't any big bugs.
This commit finally fixes this.

Closes #1651 <#1651>

¹ Exactly which transform is somewhat confusing in the case of clip
masks, but that's SVG.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants