Skip to content

Allow long-read Giraffe to take self-loops/reversals between seeds#4953

Open
faithokamoto wants to merge 114 commits into
masterfrom
reversals
Open

Allow long-read Giraffe to take self-loops/reversals between seeds#4953
faithokamoto wants to merge 114 commits into
masterfrom
reversals

Conversation

@faithokamoto

Copy link
Copy Markdown
Contributor

Changelog Entry

To be copied to the draft changelog by merger:

  • Allow long-read Giraffe (-b hifi / -b r10) to take self-loops/reversals between seeds

Description

PR'ing this since I don't see any more how to get it to work better. It decreases the number of incorrectly mapped reads, at least.

Notable changes:

  • Snarl or node levels in zipcodes now store whether it is possible to self-loop back as two bools bit-packed into metadata fields.
    • For a forward reversal, that means leave in the forward direction, then go back and enter the same bit in the reverse direction. Reverse is opposite.
    • Old zipcodes are compatible with the new code. The new algorithm will assume that they have no reversals
  • Ziptrees store reversals using a LOOP item.
    • Effort is made to de-duplicate LOOPs as much as possible.
    • During traversal, whenever we hit into a LOOP we save a position using the new S_TAKE_LOOP state. When we reload this position we immediately take the loop by turning around and then keep going.
    • Simple best-so-far memorization is used for LOOP positions to avoid infinite bouncing back between each other.
  • Graph distances between seeds are saved from chaining and are propagated all the way through to setting up base-level alignment.
  • We can extract a connecting graph even if it requires traversing one of the end nodes in both directions (i.e. taking a loop). Before we forced all end nodes to become tips. That prevented taking loops through an end now. Now if necessary a copy of an end node is made and then only one copy is made into a tip.

@adamnovak adamnovak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I looked mostly at the chaining changes and not as much at the actual loop logic, but I think there has to be a neater way to bring the graph distances out of chaining than hiding each one in a struct with the index of the seed.

I would add them to what is right now the ChainWithRec structure.

That makes them harder to carry through when we translate to chains of seeds and not chains of anchors. But maybe we could have a new structure for that?

Ultimately they really need to be available to get_graph_distance() to accelerate it, right? If they're too hard to bring everywhere with the chains, we could try caching them all in a table during chaining and giving the table to get_graph_distance().

Comment thread src/algorithms/chain_items.hpp Outdated
Comment thread src/algorithms/chain_items.hpp
Comment thread src/algorithms/chain_items.hpp
Comment thread src/algorithms/chain_items.hpp Outdated
Comment thread src/algorithms/extract_connecting_graph.hpp Outdated
Comment thread src/minimizer_mapper_from_chains.cpp Outdated
Comment thread src/zip_code.hpp Outdated
Comment thread src/zip_code.hpp Outdated
Comment thread src/zip_code_tree.cpp Outdated
Comment thread src/zip_code_tree.hpp Outdated
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