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

Edit distance #16

Merged
merged 26 commits into from
Jun 4, 2024
Merged

Edit distance #16

merged 26 commits into from
Jun 4, 2024

Conversation

poperigby
Copy link
Collaborator

@poperigby poperigby commented May 28, 2024

Supersedes #15

Help would be much appreciated for the TODO items.

TODO:

  • The test_single_character_deletion doesn't seem to work for some reason.
  • Replace use of str.chars().nth() because it makes the function O(n^2)

@poperigby
Copy link
Collaborator Author

I fixed the second problem but this will no longer work with non-ASCII characters. Not an issue in my opinion.

@learner-long-life
Copy link
Contributor

@AbyssalRemark can you check out this branch, try this code, and review it?

Copy link
Collaborator

@AbyssalRemark AbyssalRemark left a comment

Choose a reason for hiding this comment

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

Seems good to me. I think I would prefer working out that one bug before a merge but I think all round this is good to go.

}
}

impl<T> fmt::Display for Matrix<T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get why this is done this way I think but it would still be nice if we could print the grid out in context to the string so our human eyes can check it easier. But that's more then fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That didn't really show what I meant to look at very well. This was in reference to how the matrix display trait cant see the string and thus we cant print with more context. Maybe making just a printing function would be more ideal. But again, not a huge deal.

Delete,
}
/// Return the minimum amount of changes needed to transform `s1` into `s2`.
pub fn edit_distance(s1: &str, s2: &str) -> usize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might care for where changes would need to happen in the future but that has nothing to do with the quality of the code.

@learner-long-life
Copy link
Contributor

For debugging edit distance
https://phiresky.github.io/levenshtein-demo/

@learner-long-life
Copy link
Contributor

From our debugging today, it looks like there are two problems:

  • Repeated letters, as in the letter t at the end of kit vs. kitt, is currently being counted as 0 edit distance incorrectly (it should be 1)
  • Matching a letter, like n at the end of kittn and kitten after a deletion (or perhaps any mismatch).

@poperigby poperigby merged commit a4cc8f8 into main Jun 4, 2024
1 check failed
@poperigby poperigby deleted the edit-distance branch June 4, 2024 20:00
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.

None yet

3 participants