Refactor#16
Open
sarahsummerfield wants to merge 6 commits into
Open
Conversation
Hash implementation started, using the Rust-Bio library for reading in the DCE fasta file (temporarily, until our implementation has been debugged). This required refactoring of the fasta.rs file, which has been renamed to seqloader to resolve shared name conflicts with Rust-Bio. The DCE hash was implemented using the multimap library, which is a small wrapper around the hashmap that allows us to put multiple values (IDs) in for each sequence without additional handling. As such, support of non-unique sequences has already been added. These changes (fasta reading/moving away from the Seq struct) still need to be implemented for the RNAseq file, as well as changes to Search to support checking against the DCE hash. And yes... main is (temporarily) a bit of a mess.
Added line breaks in the RNA file sequences to more closely align with the format of dbGaP RNAseq files and allow to test the parsing of the fasta file correctly across line breaks.
Perform the search while continuing to use the Seq struct for the RNA sequences, but using a hash (instead of vec) for the DCEs. DCE compression is now happening at the same step as loading them from the file - they are immediately bitpacked and only stored in their bitpacked forms. The bitpacked hash is then passed into the Search function (but still currently has to be cloned, which isn't ideal). This enables just checking to see if the current RNA 32mer exists in the hash instead of using a needle index to walk through the vector. It's "working" right now, except when it finds the first hit it gets stuck in an infinite loop instead of stepping forward to the next RNA 32mer...
glesica
reviewed
Jun 2, 2020
| TGCGTTCTCCTCAGCACAGACCCGGAGAGCACCGCGAGGGCGGAGCTGCGTTCTCCTCTGCACAGATTTCGGTGGTTTCCATTTCGCTTCCTCTGGCTGCTCTCTCGAACTTTAGAAGCCCAGGAACTAGGAACCTACCCACAGATACACGT | ||
| TGCGTTCTCCTCAGCACAGACCCGGAGAGCACCGCGAGGGCGGAGCTGCGTTCTCCTCTGCA | ||
| CAGATTTCGGTGGTTTCCATTTCGCTTCCTCTGGCTGCTCTCTCGAACTTTAGAAGCCCAGG | ||
| AACTAGGAACCTACCCACAGATACACGT |
Contributor
There was a problem hiding this comment.
Is there any particular reason that these changed? Just want to understand what I'm looking at; no reason they can't change.
glesica
reviewed
Jun 2, 2020
| /// TODO: Handle shorter sequences? | ||
| pub fn from_seq(seq: &Seq, alphabet: &HashMap<char, u64>) -> Option<CompressedSeq> { | ||
| if seq.length != 32 { | ||
| pub fn from_seq(seq: &String, alphabet: &HashMap<char, u64>) -> Option<CompressedSeq> { |
Contributor
There was a problem hiding this comment.
Don't change it here, but we should make this return a Result instead so that we can include an error message.
Contributor
There was a problem hiding this comment.
I made it an Option because I was lazy :-)
glesica
reviewed
Jun 2, 2020
| /// TODO: Handle shorter sequences? | ||
| pub fn from_seq(seq: &Seq, alphabet: &HashMap<char, u64>) -> Option<CompressedSeq> { | ||
| if seq.length != 32 { | ||
| pub fn from_seq(seq: &String, alphabet: &HashMap<char, u64>) -> Option<CompressedSeq> { |
Contributor
There was a problem hiding this comment.
Another thought, you can create type aliases in Rust, so for clarity we could do something like type Sequence = String; and then this function could take a Sequence but the abstraction wouldn't cost us anything. See https://doc.rust-lang.org/reference/items/type-aliases.html
glesica
reviewed
Jun 2, 2020
| use std::io::BufWriter; | ||
| use std::io::Write; | ||
| use std::path::Path; | ||
| use bio::io::fasta; |
Contributor
There was a problem hiding this comment.
Does Optimize Imports in CLion tidy this up a bit?
glesica
reviewed
Jun 2, 2020
| let mut writer = BufWriter::new(&output); | ||
| let mut _writer = BufWriter::new(&output); | ||
|
|
||
| //read DCEs in to hash structures |
Contributor
There was a problem hiding this comment.
This is fine for now, but we ought to move this code to its own module before we're done.
glesica
reviewed
Jun 2, 2020
| let compressed_seq = CompressedSeq::from_seq(&seq, &alphabet_map).unwrap(); | ||
|
|
||
| let needles: Vec<_> = SeqLoader::from_path(Path::new(&_dna_file)).collect(); | ||
| needles.insert(compressed_seq.sequence, record.id().to_string()); |
Contributor
There was a problem hiding this comment.
If CompressedSeq isn't going to have any other fields or methods then we might as well use an alias for it and then just have a module-level function that takes a Seq (aliased to String) and an alphabet and returns a CompressedSeq (aliased to String or whatever).
glesica
reviewed
Jun 2, 2020
| println!( | ||
| "{} found in {} at offset {}. Total hits: {}", | ||
| result.needle, result.haystack, result.offset, result.hits | ||
| "{:?} found in {} at offset {}", |
Contributor
|
This looks good, needs a tiny bit of cleanup, but that can come later. I had some questions, as well. |
Infinite loop problem fixed by moving the incrementing step in Search earlier in the code. Hits tracking added in main, using the secondary hash to keep track of total hits for each DCE. Because non-unique sequence support was already added by using the MultiMap structure, this enables hit tracking totals for each separate ID that shares the same sequence.
Merged with George's branch for improving fasta reader speed. Code in search dealing with the hash is commented out, leaving the work to be done just stepping through the RNAseq 32mers. Timers have been implemented to time the hashing speed and RNA reading speed exactly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.