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

Replacing C++ with Rust #424

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Replacing C++ with Rust #424

merged 1 commit into from
Dec 17, 2019

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Feb 26, 2018

This PR replaces the C++ parts of sourmash with Rust.

Tests are passing, but some optimization might be good (especially on avoiding too many calls/memory copies across the FFI layer).

Other branches/PRs depending on this one:

  • Moving loading and save sigs to Rust #532 starts moving more of the Python implementation to Rust: signature loading and saving. This removes the dependency on ijson, but I decided to keep it separated from this PR because the Python signature loading/saving code is fine (but I needed it implemented in the Rust side for the webassembly parts).
  • There is a nodegraph implementation in Rust, exposed to Python in the rust_nodegraph branch. Still need to fix and test more before turning into the default, but would also mean optional dependency on khmer.

There are more details in https://github.com/luizirber/2018-python-rust (the poster presented at BOSC and SciPy 2018).

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@luizirber luizirber force-pushed the core/rust branch 6 times, most recently from 049e518 to d78a456 Compare February 27, 2018 00:38
@luizirber luizirber force-pushed the core/rust branch 5 times, most recently from 7f3a1d9 to 4c7855d Compare March 14, 2018 01:40
@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #424 into master will increase coverage by 1.07%.
The diff coverage is 89.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   78.52%   79.59%   +1.07%     
==========================================
  Files          43       45       +2     
  Lines        6534     6588      +54     
  Branches      499      454      -45     
==========================================
+ Hits         5131     5244     +113     
+ Misses       1100     1043      -57     
+ Partials      303      301       -2
Impacted Files Coverage Δ
src/index/bigsi.rs 60.86% <100%> (+1.31%) ⬆️
sourmash/sig/__main__.py 94.45% <100%> (-0.41%) ⬇️
sourmash/sourmash_args.py 96.73% <100%> (+0.01%) ⬆️
sourmash/sbtmh.py 85% <100%> (+0.1%) ⬆️
sourmash/__init__.py 100% <100%> (ø) ⬆️
sourmash/_compat.py 100% <100%> (ø)
src/index/sbt/mhbt.rs 41.37% <40%> (-0.16%) ⬇️
src/sketch/minhash.rs 37.92% <41.66%> (+0.2%) ⬆️
src/signature.rs 37.58% <66.66%> (+1.52%) ⬆️
sourmash/utils.py 71.92% <71.92%> (ø)
... and 7 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 93b9b90...320b021. Read the comment docs.

@luizirber
Copy link
Member Author

Turns out there is only one place that uses the common set returned by intersection: the test method =P
ccc05ec#diff-acb3fd27a7c58bb1a046d1a231f2d6cdR277

While we don't have anything depending on this behavior I suggest we add an in_common argument (defaulting to False) to avoid having to build and output the common set.
ccc05ec#diff-2032759ae736988ba55922787626efb5R281
(I still need to do the rust wrapping around the common set, that's why it is a weird mixed implementation for now)

@ctb
Copy link
Contributor

ctb commented Mar 15, 2018 via email

@luizirber luizirber mentioned this pull request Mar 16, 2018
5 tasks
@luizirber
Copy link
Member Author

@ctb: covered the intersection changes in #453

@luizirber luizirber force-pushed the core/rust branch 3 times, most recently from 369b8af to 271b3ee Compare June 29, 2018 00:13
@luizirber luizirber added the rust label Aug 16, 2018
@luizirber luizirber force-pushed the core/rust branch 2 times, most recently from d32e2fc to 3b3c533 Compare October 5, 2018 02:03
@luizirber luizirber force-pushed the core/rust branch 4 times, most recently from 4b6bf18 to d0b26de Compare December 7, 2018 21:16
@luizirber luizirber force-pushed the core/rust branch 19 times, most recently from 5104d1a to 6e76c87 Compare December 17, 2019 03:54
@luizirber luizirber changed the title [MRG] Replacing C++ with Rust Replacing C++ with Rust Dec 17, 2019
- Update build system and CI
- Add Rust install instructions to docs
- Remove dependency on Cython (replaced with cffi and milksnake)
- Move _minhash.pyx to _minhash.py, and remove Cython bits
- Add convenience functions and classes to work with Rust layer
- Remove third-party/ directory
@luizirber
Copy link
Member Author

Ready for review and merge @ctb

@luizirber luizirber requested a review from ctb December 17, 2019 04:58
@luizirber luizirber merged commit 3febb3f into master Dec 17, 2019
@luizirber luizirber mentioned this pull request Dec 17, 2019
5 tasks
@olgabot
Copy link
Collaborator

olgabot commented Dec 18, 2019

OMG WHOO!!!! Congrats!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants