Conversation
nguyen-mai-huong
left a comment
nguyen-mai-huong
left a comment
There was a problem hiding this comment.
This PR will be closed because the way the code was presented showed little consideration for reviewers to work on.
Firstly, it's hard to keep track of code flow. Secondly, it is not based on the latest master which results in some problem.
Please help make the code flow easier to understand, and rebase the code on the latest master before pushing to another PR.
| } | ||
| s1RegionTokens.add(s1Tokens.get(i)); | ||
| if (i <= s1EndIndex - nGramSize + 1) { | ||
| if (i <= match_s1_end - nGramSize + 1) { |
There was a problem hiding this comment.
variable name did not follow Java naming convention.
| s2StartIndex = m.getStartIndex2(); | ||
| s2EndIndex = m.getEndIndex2(); | ||
| int idx2 = m.getStartIndex2(); | ||
| int match_s2_end = m.getEndIndex2(); |
There was a problem hiding this comment.
similarly, variable name did not follow Java naming convention.
| private boolean all_is_unmarked(NGramList l, int start, int end) { | ||
| boolean ans = true; | ||
|
|
||
| if (l.equals(l1) ) { |
There was a problem hiding this comment.
Thanks for the comments. Variables' names are important to help keep track of code flow. The code presented here is not friendly for reviewers to understand.
| * | ||
| * @param m size of window | ||
| */ | ||
| public NGramHasher(int m) { |
There was a problem hiding this comment.
Not recommended way of naming variables.
There was a problem hiding this comment.
This PR will be closed because the way the code was presented showed little consideration for reviewers to work on.
The code flow's readability needs to be improved. Please help make the code flow more readable and understandable, and rebase the code on the latest master before pushing to another PR.
The original intention of this PR is something of a proof-of-concept to test whether or not the speedup of RKR-GST in practical setting is worth pursuing. The fact that it was not fully integrated with latest master (currently blocked by differences in hashing component of winnowing implementation and modified version in this implementation) was mentioned in 22 May meeting (albeit could have been clearer). In any case, I am working on the integration with latest master, so I agree with testing after the integration and addressing other comments. |
2 participants
This PR aims to implement RKR-GST for SSID