Improve Read Cycle Layout #389

ducku · 2024-01-28T23:16:52Z

Segments cycling on the first node had strange interactions since segments classified as "outgoing"(if its pathIndex == 0) had different methods of sorting.

Changes are made to how reads are placed outside of nodes when a cycle is detected, and inside of nodes when reads have the same source and destination Node.

A new tiebreaker is placed in sortByGoingTo for reads that start and end in the same node. Now they're placed based on the endmost node they were placed in, working its way back if there are no y values. Since these reads start and end in the same node, this should start the reads(in the first node) in the same order as the other nodes.
Segments outside of nodes now detects cycles, defined as the previous and next segments are in the node and going in the same direction. Segments outside of nodes and are between cycles now sort in reverse to keep the layout clean.
Extra space in between nodes are calculated after reads are added into the tracks variable.

Closes #321

…ing segments

…generating horizontal space between nodes more accurate

adamnovak

This looks pretty good, and way better than before.

But I'm concerned that it's specialized for one-node cycles, when probably we need to support cycles of multiple nodes.

Also, what's going on in the first screenshot with the innermost three reads in the spiral below the node? They don't seem to be spiraling in the same order as the other ones, and so they end up crossing over each other for no apparent reason. It's not a big problem for these 3 reads, but I think a similar problem is happening with more reads in the second screenshot, and it makes the visualization messier there.

adamnovak · 2024-01-29T22:17:39Z

src/util/tubemap.js

@@ -682,10 +684,32 @@ function placeReads() {
          previousValidY = reads[idx].path[lastIndex].y;
          lastIndex = lastIndex - 1;
        }
+
+        // sometimes, elements without nodes are between 2 segments going to the same node, from the same direction
+        // this means we're looping back to the same node, which constitutes a different sorting priority


I'm not sure "constitutes" is the right word here. Do you mean we want to use a different sorting in this case?

src/util/tubemap.js

adamnovak · 2024-01-29T22:22:07Z

src/util/tubemap.js

+        // if the previous segment and the next segment is going to the same node
+        if (sameNode && sameDirection) {
+          betweenCycle = true;
+        }


I think we might want to use this reverse sort order logic in more cases than just the one where we visit the same node in the same orientation before and after the no-node section.

I think we want to have the same behavior even if that one node that we loop around is instead divided up into two or more nodes that we loop around as a group. Like if the read visits nodes 1, 2, 1, 2, 1, 2. I'm not sure of the best way to detect that we are visiting no node because we need to get back to an earlier X to complete a cycle, though.

ducku · 2024-02-04T21:17:17Z

Changing to sort the first segment by existing y values first works better on this set of data, and I think it would work better overall.

I'm currently trying to make an example dataset showcasing this.

adamnovak · 2024-02-05T15:14:09Z

That's looking pretty good!

I guess in the second image it would be better if those reads on the inside coming down stayed on the inside when turning to go left, instead of going all the way down and coming left on the outside. But this is much cleaner than before.

adamnovak

This looks like it ought to work, but I think some parts of the code could be simplified and some of the comments and variable names could be made clearer.

adamnovak · 2024-02-12T21:54:26Z

src/util/tubemap.js

        let lastIndex = pathIdx - 1;
-        while (previousValidY === null && lastIndex >= 0) {
-          previousValidY = reads[idx].path[lastIndex].y;
+        let previousPathWithNode;


I don't think this is a "path". The things in reads[idx].path[] are visits or path entries, so this should be named something more like previousVisitToNode. Or maybe previousVisitToAnyNode? Since it's not always to this node. Maybe just previousVisit?

adamnovak · 2024-02-12T21:55:04Z

src/util/tubemap.js

+
+        // Find the next node in our path
+        let nextPathIndex = pathIdx + 1
+        let nextPathWithNode = reads[idx].path[nextPathIndex];


This would also make more sense as something like nextVisit?

adamnovak · 2024-02-12T21:59:23Z

src/util/tubemap.js

+          let previousNodeIsForward = previousPathWithNode?.isForward;
+          let nextNodeIsForward = nextPathWithNode?.isForward;
+
+          // if the next segment(to a node) and previousl segment(from a node) is going in the same direction


Suggested change

// if the next segment(to a node) and previousl segment(from a node) is going in the same direction

// if the next segment(to a node) and previous segment(from a node) are going in the same direction

adamnovak · 2024-02-12T22:00:35Z

src/util/tubemap.js

+          let sameDirection = previousNodeIsForward !== null && nextNodeIsForward !== null && previousNodeIsForward === nextNodeIsForward
+
+          // we're visiting the same node we just visited from the same direction
+          if (sameDirection) {


Shouldn't we just put the conditions in the ifs, if we're only going to fill in the variable to immediately condition on it? It's useful to have the variable name for documentation, but we could also just have a comment.

adamnovak · 2024-02-12T22:08:11Z

src/util/tubemap.js

+        let nextNodeOrder = nextPathWithNode?.order;
+        let previousNodeOrder = previousPathWithNode?.order;
+
+        // if the previous segment and the next segment is going to the same node
+        let nextNodeJustVisited = previousNodeOrder !== null && nextNodeOrder !== null && previousNodeOrder === nextNodeOrder;
+
+
+        // a segment can also be in a loop if the next node is behind on the map
+        let nextNodeBehind = previousNodeOrder !== null && nextNodeOrder !== null && previousPathWithNode.order > nextNodeOrder;
+
+
+        reads[idx].path[pathIdx].betweenCycle = false;
+        if (nextNodeBehind) {
+          // the segment has to loop regardless of which direction the read goes in
+          reads[idx].path[pathIdx].betweenCycle = true;
+        } else if (nextNodeJustVisited) {
+          // the segment doesn't have to loop if going in opposite direction from the node
+          let previousNodeIsForward = previousPathWithNode?.isForward;
+          let nextNodeIsForward = nextPathWithNode?.isForward;
+
+          // if the next segment(to a node) and previousl segment(from a node) is going in the same direction
+          let sameDirection = previousNodeIsForward !== null && nextNodeIsForward !== null && previousNodeIsForward === nextNodeIsForward
+
+          // we're visiting the same node we just visited from the same direction
+          if (sameDirection) {
+            reads[idx].path[pathIdx].betweenCycle = true;
+          }
+        }


All this stuff is one big boolean function to fill in betweenCycles; I feel like it might be able to be simplified somehow.

If either side doesn't exist or has order (or isForward) not set, the answer is false. Otherwise, we are true if the orders and the orientations are equal, or if the previous order exceeds the next order.

You could turn that into one expression with comments. If we don't have to deal with real nulls in there, that would be something like:

// If there is a next and previous, we can be between cycles let betweenCycles = (nextPathWithNode && previousPathWithNode && // If we go back to the same node in the same orientation ((nextPathWithNode.order == previousPathWithNode.order && nextPathWithNode.isForward == previousPathWithNode.isForward) || // Or we go back to a previous node in any orientation (nextPathWithNode.order < previousPathWithNode.order)));

adamnovak · 2024-02-12T22:09:00Z

src/util/tubemap.js

+        let previousNodeOrder = previousPathWithNode?.order;
+
+        // if the previous segment and the next segment is going to the same node
+        let nextNodeJustVisited = previousNodeOrder !== null && nextNodeOrder !== null && previousNodeOrder === nextNodeOrder;


Are there actual nulls stored in order and/or isForward on the visits? Or should these be checking for undefined from the optional chaining ?. instead?

adamnovak · 2024-02-12T22:20:19Z

src/util/tubemap.js

+    // we want to sort of the reverse when the segment is between a cycle
+    // this ensures a loop that starts on the outside, stays on the outside
+    // and loops are rolled in order


This comment could be better written, I think. Maybe:

Suggested change

// we want to sort of the reverse when the segment is between a cycle

// this ensures a loop that starts on the outside, stays on the outside

// and loops are rolled in order

// We want to sort in reverse order when the segment is along the reverse-going part of a cycle.

// This ensures a loop that starts on the outside, stays on the outside,

// and rolls up in order with other loops.

adamnovak · 2024-02-12T22:22:35Z

src/util/tubemap.js

+        let nextNodeBehind = previousNodeOrder !== null && nextNodeOrder !== null && previousPathWithNode.order > nextNodeOrder;
+
+
+        reads[idx].path[pathIdx].betweenCycle = false;


I don't know if the idea of "being between a cycle" means anything to me. All the visits along the path that the read takes from a node back to the same node "participate in" the cycle, but here we're specifically interested in the visits where we're in the part of the cycle that goes from right to left when laid out, I think, right? Is there another way to say this?

adamnovak · 2024-02-12T22:27:01Z

src/util/tubemap.js

 function compareReadOutgoingSegmentsByGoingTo(a, b) {
+  // Expect two arrays both containing 2 integers.
+  // The first index of each array contains the read index
+  // The second index of each array contains the path index
  let pathIndexA = a[1];
  let pathIndexB = b[1];
-  // let readA = reads[a[0]]
-  // let nodeIndexA = readA.path[pathIndexA].node;
-  let nodeA = nodes[reads[a[0]].path[pathIndexA].node];
-  let nodeB = nodes[reads[b[0]].path[pathIndexB].node];
+  let readIndexA = a[0];
+  let readIndexB = b[0];


Can't we use argument destructuring here?

Suggested change

function compareReadOutgoingSegmentsByGoingTo(a, b) {

// Expect two arrays both containing 2 integers.

// The first index of each array contains the read index

// The second index of each array contains the path index

let pathIndexA = a[1];

let pathIndexB = b[1];

// let readA = reads[a[0]]

// let nodeIndexA = readA.path[pathIndexA].node;

let nodeA = nodes[reads[a[0]].path[pathIndexA].node];

let nodeB = nodes[reads[b[0]].path[pathIndexB].node];

let readIndexA = a[0];

let readIndexB = b[0];

function compareReadOutgoingSegmentsByGoingTo([readIndexA, pathIndexA], [readIndexB, pathIndexB]) {

ducku added 5 commits January 27, 2024 15:18

sort outgoing read segment now tiebreaks by y value placed from incom…

71a7044

…ing segments

remove logs

c6c28df

GenerateNodeXCoords after reads are added to tracks variable, making …

5445419

…generating horizontal space between nodes more accurate

place read segments between cycles in reverse previousY sorting order

f8042b2

comments and TODO

0b829d3

adamnovak requested changes Jan 29, 2024

View reviewed changes

improve cycle detection logic and outgoing sort priority

da1f090

ducku and others added 4 commits February 10, 2024 17:23

account for previous path in a read not being a node in cycle detections

5d84648

Multiple Nodes Cycle Example

2c75e70

Segment cycle detection improvement and new exmaple data

a9554eb

Drop dead code

f409bd4

adamnovak requested changes Feb 12, 2024

View reviewed changes

ducku and others added 2 commits February 18, 2024 17:46

Syntax and logic improvements

f1750c1

Merge commit 'f1750c1' into issues/321-improve-read-cycle-layout

5911a99

adamnovak approved these changes Feb 26, 2024

View reviewed changes

adamnovak merged commit e23ffed into vgteam:master Feb 26, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Read Cycle Layout #389

Improve Read Cycle Layout #389

ducku commented Jan 28, 2024

adamnovak left a comment

adamnovak Jan 29, 2024

adamnovak Jan 29, 2024 •

edited

Loading

ducku commented Feb 4, 2024

adamnovak commented Feb 5, 2024

adamnovak left a comment •

edited

Loading

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

adamnovak Feb 12, 2024

	// if the next segment(to a node) and previousl segment(from a node) is going in the same direction
	// if the next segment(to a node) and previous segment(from a node) are going in the same direction

		let nextNodeBehind = previousNodeOrder !== null && nextNodeOrder !== null && previousPathWithNode.order > nextNodeOrder;


		reads[idx].path[pathIdx].betweenCycle = false;

Improve Read Cycle Layout #389

Improve Read Cycle Layout #389

Conversation

ducku commented Jan 28, 2024

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

ducku commented Feb 4, 2024

adamnovak commented Feb 5, 2024

adamnovak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak Jan 29, 2024 •

edited

Loading

adamnovak left a comment •

edited

Loading