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

Calculate ANI based on a CLI flag #164

Closed
mr-eyes opened this issue Dec 17, 2023 · 8 comments
Closed

Calculate ANI based on a CLI flag #164

mr-eyes opened this issue Dec 17, 2023 · 8 comments
Labels

Comments

@mr-eyes
Copy link
Member

mr-eyes commented Dec 17, 2023

I think incorporating ANI calculation in the branchwater plugin will be very useful. For performance, the code will need to be rewritten in Rust, though.

@mr-eyes
Copy link
Member Author

mr-eyes commented Jan 18, 2024

Hi @ctb and @bluegenes

I wrote a Rust implementation of the sourmash ANI estimation code many weeks ago but never committed it 3b1c1b8

Just putting it here for now in case it's useful until I can continue the integration later, or maybe someone faster than me will do.

The Rust implementation is 712X faster than the original Python implementation.

@ctb
Copy link
Collaborator

ctb commented Jan 18, 2024

The Rust implementation is 712X faster than the original Python implementation.

not good enough. Do better.

😆 that's awesome!

@bluegenes
Copy link
Contributor

nice!!! I was just thinking I needed to write one so I could add ANI into fastmultigather results 😂

@mr-eyes
Copy link
Member Author

mr-eyes commented Jan 18, 2024

nice!!! I was just thinking I needed to write one so I could add ANI into fastmultigather results 😂

Let's coauthor a PR to add this to all commands (I hope you do the testing 😂).

@ctb
Copy link
Collaborator

ctb commented Jan 19, 2024

Let's coauthor a PR to add this to all commands (I hope you do the testing 😂).

🤔

@mr-eyes
Copy link
Member Author

mr-eyes commented Jan 19, 2024

Let's coauthor a PR to add this to all commands (I hope you do the testing 😂).

🤔

I am not very skilled at writing tests in sourmash because it requires a broader context that I am not very familiar with.

@bluegenes
Copy link
Contributor

bluegenes commented Feb 24, 2024

Streamlined ANI from sourmash-bio/sourmash#2943 seems to be 2x faster than this rust implementation, as it doesn't cause a 2x slowdown compared to not calculating ANI :)

The benchmark here was a strong motivator for the streamlining work. So yay!

comparison benchmark (ANI vs not): #236 (comment)

streamlined ani_utils are here: https://github.com/sourmash-bio/sourmash/blob/latest/src/core/src/ani_utils.rs

@ctb
Copy link
Collaborator

ctb commented Jun 20, 2024

Remaining functionality added in #298 and #361. ANI should now be available everywhere ;)

@ctb ctb closed this as completed Jun 20, 2024
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 a pull request may close this issue.

3 participants