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

implement one row linear viewer #205

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

bugzpodder
Copy link
Collaborator

@bugzpodder bugzpodder commented Apr 27, 2023

@jjti
Copy link
Collaborator

jjti commented May 6, 2023

Seeing one small thing @bugzpodder where the annotation wrapping edges are showing up along the linear viewer like:

Screenshot from 2023-05-06 15-58-25

That should be fixable down in Annotations around among all this nonsense for edges and stuff: https://github.com/Lattice-Automation/seqviz/blob/develop/src/Linear/Annotations.tsx#L130-L182

Comment on lines 168 to 178
blockWidths[i] = blockWidth;
if (blockHeight > maxBlockHeight) {
maxBlockHeight = blockHeight;
}
if (maxTranslationRowSize < translationRows[i].length) {
maxTranslationRowSize = translationRows[i].length;
}
if (maxAnnotationRowSize < annotationRows[i].length) {
maxAnnotationRowSize = annotationRows[i].length;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've only skimmed this so far -- so I'm not saying this with certainty -- but do we need to calculate the max at all @bugzpodder. I would think that down in SeqBlock we could just use the size of the first (and only) row every time

Copy link
Collaborator Author

@bugzpodder bugzpodder May 7, 2023

Choose a reason for hiding this comment

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

The reason for using max is actually for exactly this reason:
#205 (comment)

The bps and the yellow bar might not match up because of differing translation/annotation. See this screenshot.

Screenshot 2023-05-06 at 8 56 52 PM

This sequence is what's being used in the current demo
At bps 1170 you'll see the amino acid translation taking two rows. If you feel this is ok then then I can take out the maxTranlsationRowSize/maxAnnotationRowSize calculations. Now that I think of it, what I have today still doesn't guarentee that everything lines up. If you have two translations in the first block, and second translation extends to the second block, you'll still get a broken translation block which is not ideal.

In case you are curious the reason why they don't match up in this particular sequence is because of the translations prop from the demo:

    translations: [
      { direction: -1, end: 630, start: 6 },
      { end: 1147, start: 736 },
      { end: 1885, start: 1165 },
    ],

Note that here the second translation section and third section doesn't actually overlap (1147/1165) but they happen to be in the same SeqBlock but the code did not figure out that they happen to be on the same row.

@bugzpodder
Copy link
Collaborator Author

bugzpodder commented May 7, 2023

Looks a bit better but still not ideal. not too familiar with SVG but ideally can make the edge invisible or same as background color but couldn't really figure it out.
Screenshot 2023-05-06 at 11 48 14 PM

@jjti
Copy link
Collaborator

jjti commented May 7, 2023

@bugzpodder I'm toying around with something like what's in this example PR's commit: 95fa062

We'd literally have only a single SeqBlock that covers the entire sequence.

Some pros/cons of that different approach in that commit above:

Pros:

  • can re-use existing InfiniteScroll (just one row)
  • we don't have to worry about jagged annotation edges/translation edges/etc. Lots of elements account for the edges of SeqBlocks, so all will have to have modified rendering if the oneRow case
    image
  • pretty minimal changes to existing linearProps and no need for the max* variables where we build up the maximum across all blocks (because there's only one block)

Cons:

  • new logic needed for scrolling left/right on selection
    • eg if we click base 1000 on circular, this should scroll the single row viewer to bp 1000 on the single row viewer
  • rendering is extremely slow. Selecting ranges of sequences is unacceptable slow, 3-5sec latency. We need to add in some logic to check for and avoid rendering anything outside the range of the .la-vz-seqblock-container (which I haven't done in that example commit)
    • I was thinking maybe we could put it in SeqBlock.findXAndWidth... like maybe it returns:
findXAndWidth(firstIndex: number, lastIndex: number): {width: number, x: number, render: boolean}

where render is false if the passed first/lastIndex are way outside the viewer's current viewer, and the corresponding component shouldn't bother rendering at all for performance reasons. For example, in Linear/Index.tsx, we'd skip rendering ticks if we see the ticks index is so far outside the Viewer that it'd never show up anyway like:

    return tickIndexes.map(p => {
      let { x: tickFromLeft, render } = findXAndWidth(p - 1, p - 1); // for midpoint
      if (!render) {
        return null;
      }
      tickFromLeft += charWidth / 2;

I'm open to other thoughts/opinions on this. And I realize it's a bit of a drift in approach from the PR, but I think it might be less total work...

@bugzpodder
Copy link
Collaborator Author

I think that's a really neat idea. If we can render using just a partial block and do some optimizations so the svg path are only drawn for whats in the viewport that would obviously be best.

definitely prefer that since its much easier to reason with and code.

@leshane
Copy link
Contributor

leshane commented Sep 16, 2023

This is awesome. Looks like we've made some progress with this PR. Can we carry this over the finish line? @jjti @bugzpodder

@bugzpodder
Copy link
Collaborator Author

@leshane I think @jjti is planning a simpler implementation. The issue with my version is that when two blocks are connected together you see some weird artifacts: #205 (comment)

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.

Add third viewer: a one-row Linear Genome Overview
3 participants