Aho corasick#102
Closed
pscheil wants to merge 10 commits intocbielow:developfrom
Closed
Conversation
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
| { | ||
| UInt32 base :22 ; // base value of DA | ||
| UInt32 lcheck :8 ; // arc label from parent node | ||
| UInt32 leaf_flag :1 ; // node is leaf |
Owner
There was a problem hiding this comment.
one could play around with the order of the members here, and see what performs the best (since extraction of a single bit is not cheap) and extraction of lcheck could be cheaper if moved to the back...
(keep in mind for later maybe)
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
cbielow
reviewed
Aug 27, 2020
Owner
cbielow
left a comment
cbielow
left a comment
There was a problem hiding this comment.
I did not get to the end. will finish later :)
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
| { | ||
| bc_[node_pos].leaf_flag = 1; | ||
| bc_[node_pos].base = static_cast<UInt32>(tail_.size()); | ||
| storeInTail_(sequences_[unique_idx_[begin]].substr(depth)); |
Owner
There was a problem hiding this comment.
substr() is expensive.
try a StringView
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Aug 30, 2020
cbielow
reviewed
Sep 21, 2020
cbielow
reviewed
Sep 21, 2020
cbielow
reviewed
Sep 21, 2020
| bool failure_(); | ||
|
|
||
| /** | ||
| * @brief returns the supply link if this does not eliminate ambiguous aa. Don't do anything special with 0. Must be considered when calling |
Owner
There was a problem hiding this comment.
this function is only useful for spawns, i.e. when dealing with ambAA's, right?
So should there be a sibling function
void followSupplyLink(Int32 from, Int32& to)?
Can you also make input=output, i.e. merge the first two parameters?
cbielow
reviewed
Sep 21, 2020
cbielow
reviewed
Sep 21, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
| { | ||
| Int32 node; ///< Node after the ambiguous aa transition | ||
| UInt32 prot_pos; ///< Protein position after the ambiguous aa | ||
| uint8_t counter; ///< Count of all read ambiguous aa |
Owner
There was a problem hiding this comment.
so, this is the number of aa's in the path from root to 'node'?
cbielow
reviewed
Nov 24, 2020
| UInt32 aa_count_ = 0; | ||
|
|
||
| /// Indices of the sorted sequences, duplicate sequences are removed | ||
| std::vector<UInt32> unique_idx_{}; |
cbielow
reviewed
Nov 24, 2020
| void buildTrie_(Size begin, const Size end, const Size depth, const UInt32 node_pos); | ||
|
|
||
| /// returns base value | ||
| UInt32 xCheck_(); |
cbielow
reviewed
Nov 24, 2020
| OPENMS_LOG_INFO << "Mapping " << pep_DB.size() << " peptides to " << (proteins.size() == PROTEIN_CACHE_SIZE ? "? (unknown number of)" : String(proteins.size())) << " proteins." << std::endl; | ||
|
|
||
| if (length(pep_DB) == 0) | ||
| if (pep_DB.size() == 0) |
cbielow
reviewed
Nov 24, 2020
| AhoCorasickDA fuzzyAC(pep_DB); | ||
| auto s2 = std::chrono::high_resolution_clock::now(); | ||
| std::chrono::duration<double,std::milli> elapsed = s2 - s1; | ||
| std::cout << "Construction time " << elapsed.count() << " ms"<< std::endl; |
Owner
There was a problem hiding this comment.
why the extra chrono?
StopWatch is a high-resolution clock
(just don't convert to int() as done below to retain its accuracy)
cbielow
reviewed
Nov 24, 2020
| // CDA | ||
| //------------------------------------------------------------------------------------------------------------------------------------- | ||
|
|
||
| UInt32 AhoCorasickDA::stat() |
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
cbielow
reviewed
Nov 24, 2020
|
|
||
| #include <OpenMS/ANALYSIS/ID/AhoCorasickAmbiguous.h> | ||
| //#include <OpenMS/ANALYSIS/ID/AhoCorasickAmbiguous.h> | ||
| #include <OpenMS/ANALYSIS/ID/AhoCorasickDA.h> |
Owner
There was a problem hiding this comment.
can you get rid of the seqan includes?
cbielow
reviewed
Nov 24, 2020
| namespace OpenMS | ||
| { | ||
|
|
||
| /** |
Owner
There was a problem hiding this comment.
can you describe briefly some of the special tweaks here?
e.g. duplicate sequences are handled externally; how internal end nodes are handled; how spawns are processed (at what time and in what order), .... so the user gets a rough idea of what to expect here
cbielow
reviewed
Nov 24, 2020
| struct BCNode_ | ||
| { | ||
| UInt32 base :22 ; ///< base value of the node | ||
| UInt32 lcheck :8 ; ///< arc label from parent node. Only code of the aa is stored |
Owner
There was a problem hiding this comment.
it might be faster if you switch the order of base and lcheck because that should(!) save some bitshifting when accessing lcheck .. but it might not help....
don't redo the benchmarks!
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.
Description
Please include a summary of the change and which issue is fixed.
Also please:
Checklist: