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

Extra Space Between Nodes #400

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

ducku
Copy link
Collaborator

@ducku ducku commented Mar 10, 2024

Sometimes tracks don't have enough space to make a smooth transition from node to node. This PR creates extra space between nodes, taking into account the maximum difference in y values between nodes given any track.

snp1kg-BRCA1
Before:
snp1kg-BRCA1_before

After:
snp1kg-BRCA1_after

chr1_15608265_15608765
Before:
chr1_15608265_15608765_before

After:
chr1_15608265_15608765_after

@adamnovak
Copy link
Member

I like the space, but now it's easier to see that the reads are just sort of free-form swooshing around in these gaps, and their lines can get pretty thin along the axis perpendicular to their direction of motion. I wonder if there's a good way to use the extra space to ensure a minimum thickness or to get the reads to stick to horizontal and 45 degree angles.

@ducku
Copy link
Collaborator Author

ducku commented Mar 16, 2024

Currently, curves are drawn using SVG's Beziar Curves. We draw 2 parallel curves and fill them in.

Turns out, drawing parallel Beziar Curves are pretty difficult.

In commit 8b09fa2, some simple numbers were tweaked to make the curves look nicer.

In commit 14a46ec, I attempted to group sets of curves together and skew them based on how they're traversing. I think the results are better, but I'm not sure if it's the right way to do this. Maybe it's not a commit we want to keep.

snp1kg-BRCA1:
Skewed_Beziar_snp1kg-BRCA1

ch1_15608265_15608765:
Skewed_Beziar_ch1_15608265_15608765

A low hanging fruit could be adding more space between nodes based on the number of curvy reads.

Some improvements can also be made to how curves are grouped. Sometimes, curves start and end on the same node, but aren't neccesarily traversing together. Other times, curves start and end on different nodes, but are traversing closely together. In snp1kg-BRCA1, there's a single read going from T that gets covered up by the reads going from C. Them being grouped together can prevent the coverup.

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the reads do narrow less now. This might be as good as we can get without doing fancy curve fitting to find parallel curves, or using some other curve representation.

I have a couple comments on the code that I think should be addressed, but otherwise I think this is good.

We might want to add a checkbox in the visualization options accordion to remove the new horizontal space if someone wants a very compact view.

if (track.path[i].order === track.path[i - 1].order) {
// repeat or translocation
if (track.path[i].isForward === true) {
leftSideEdges[track.path[i].order] += 1;
} else {
rightSideEdges[track.path[i].order] += 1;
}
} else {
// Track is going to a differnt node, account for space needed to limit rise/fall angle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Track is going to a differnt node, account for space needed to limit rise/fall angle
// Track is going to a different node; account for space needed to limit rise/fall angle

@@ -3671,8 +3688,12 @@ function drawTrackRectangles(rectangles, type, groupTrack) {
}

function compareCurvesByLineChanges(a, b) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer compares by the number of vertical lanes changed, so I think it probably needs a different name now,

@adamnovak adamnovak merged commit 60b21fd into vgteam:master Mar 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants