Skip to content

Rework Levenshtein distance #483

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

Merged
merged 15 commits into from
Apr 10, 2023

Conversation

aesteve
Copy link
Contributor

@aesteve aesteve commented Apr 4, 2023

Rework Levenshtein distance

Description

Rework a bit the Levenshtein distance implementation by using a more Rust-idiomatic way (iterators).
Add a detailed documentation on how the algorithm works.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I ran bellow commands using the latest version of rust nightly.
  • I ran cargo clippy --all -- -D warnings just before my last commit and fixed any issue that was found.
  • I ran cargo fmt just before my last commit.
  • I ran cargo test just before my last commit and all tests passed.
  • I checked COUNTRIBUTING.md and my code follows its guidelines.

Please make sure that if there is a test that takes too long to run ( > 300ms), you #[ignore] that or
try to optimize your code or make the test easier to run. We have this rule because we have hundreds of
tests to run; If each one of them took 300ms, we would have to wait for a long time.

@aesteve aesteve requested review from siriak and imp2002 as code owners April 4, 2023 17:40
@aesteve aesteve changed the title Rework levenshtein distance Rework Levenshtein distance Apr 4, 2023
@aesteve
Copy link
Contributor Author

aesteve commented Apr 5, 2023

Also, I think there's a duplicate between:

Not sure which one should take precedence but both are implementing Levenshtein distance.

Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge edit distance file into this one and delete the original. As for your changes, could you remove comments with example from the code? Examples don't need to be included if this can be looked up on Wikiipedia

@aesteve
Copy link
Contributor Author

aesteve commented Apr 10, 2023

Thanks for the feedback.

  • I fixed the 3 reviewed items, thanks for that

As for the rest:

  • I merged edit distance into the Levenshtein distance, but kept the entry in the DIRECTORY.md under Dynamic Programming to point at the Levenshtein distance:
    • in case some people are used to look for "edit distance" in the directory maybe? so that the change is a bit backwards compatible in a sense
    • and well, because Levenshtein distance is actually dynamic programming, indeed but works on Strings. So... It kinda makes sense.

I can remove it if you want, let me know.


For the comments I removed them but I truly think this is a shame. Surely it can be looked be on Wikipedia, but they walked through the algorithm in a shorter yet understandable way.

@siriak
Copy link
Member

siriak commented Apr 10, 2023

Thanks, looks good!

It's supposed to be as close to a real-life implementation as possible so that people can not only get familiar with the algorithm, but also with the language and coding conventions. So, it's different from a book and not supposed to be a complete self-explanatory text read as a book. We don't include details that are verbose and can be googled.

It's like when using patterns, saying that we use "visitor" pattern here is enough and an explanation of what that is can be easily googled. Hope that makes sense.

@siriak siriak merged commit e01ace3 into TheAlgorithms:master Apr 10, 2023
@aesteve
Copy link
Contributor Author

aesteve commented Apr 10, 2023

Makes total sense.

I probably need to rewrite the count-min-sketch ( #485 one) and a few others I was preparing (bloom filters, hyperloglog) as well then.

aesteve added a commit to aesteve/algorithms-in-rust that referenced this pull request Apr 12, 2023
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.

2 participants