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

Mqf integration2 #1873

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Mqf integration2 #1873

wants to merge 12 commits into from

Conversation

shokrof
Copy link
Collaborator

@shokrof shokrof commented Jun 18, 2018

MQF, Mixed Quotient Filter, is a new variant of CQF. MQF has changed the counting scheme of the parent data structure by adding fixed size counters and using a different encoding for the dynamic one.
This improvement makes MQF more memory efficient for a wider range of Zipfian distributions. Also, MQF expanded the metadata component of the filter to enable tagging of its kmers. In addition to the basic functions provided by the original CQF, the new data structure comes with an expanding list of kmer processing functions including functions for merging and resizing the filter, functions for comparing kmer sets, and functions for indexing and meta-analysis of multiple kmer groups. More details about these functions can be found here

  • Is any new functionality in tested? (This can be checked with
    make clean diff-cover or the CodeCov report that is automatically
    generated following a successful CI build.)
  • Was a spellchecker run on the source code and documentation after
    changes were made?
  • Have any changes to the command-line interface been explicitly described?
    Only backwards-compatible additions are allowed without a major version
    increment. Changing file formats also requires a major version number
    increment.
  • Have any substantial changes been documented in CHANGELOG.md? See
    keepachangelog for more details.
  • Do the changes respect streaming I/O? (Are they tested for streaming I/O?)

@codecov-io
Copy link

codecov-io commented Jun 18, 2018

Codecov Report

Merging #1873 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1873      +/-   ##
=========================================
- Coverage    0.07%   0.06%   -0.01%     
=========================================
  Files          67      67              
  Lines        6685    7222     +537     
  Branches     2487     434    -2053     
=========================================
  Hits            5       5              
- Misses       6680    7217     +537
Impacted Files Coverage Δ
src/oxli/storage.cc 0% <0%> (ø) ⬆️
include/oxli/storage.hh 0% <0%> (ø) ⬆️
src/oxli/hllcounter.cc 0% <0%> (ø) ⬆️
src/oxli/traversal.cc 0% <0%> (ø) ⬆️
src/oxli/assembler.cc 0% <0%> (ø) ⬆️
src/khmer/_cpy_utils.cc 0% <0%> (ø) ⬆️
include/oxli/kmer_hash.hh 0% <0%> (ø) ⬆️
src/khmer/_cpy_khmer.cc 0% <0%> (ø) ⬆️
src/oxli/kmer_hash.cc 0% <0%> (ø) ⬆️
include/oxli/read_parsers.hh 0% <0%> (ø) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe0ce11...789f1b2. Read the comment docs.

@standage
Copy link
Member

DUUUUUUUUUDE!!!! Are those passing tests I see???

@shokrof
Copy link
Collaborator Author

shokrof commented Jun 18, 2018

Yes :)
I solved the problem by adding "-stdlib=libc++" and "-nostdinc+" while compiling my c++ library(MQF). Also, I added -lstdc++ while linking.

I am now upgrading the mqf to the latest version and the request will be ready for merging.

@shokrof
Copy link
Collaborator Author

shokrof commented Jun 19, 2018

@ctb @standage @drtamermansour
MQF is now ready to be integrated into Khmer.
Please review. I am waiting for your comments.

Have a nice day.
Mostafa

@drtamermansour
Copy link
Member

drtamermansour commented Jun 19, 2018

@shokrof I added a description to the pull request but you still need to review the pull request checklist. Also, I think we should reference and close the related pull requests #1859 #1865

@ctb
Copy link
Member

ctb commented Jul 22, 2018

hi folks, this looks ready to review - am I right?

@drtamermansour
Copy link
Member

Yes. It should be ready to go a while ago.

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.

5 participants