-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add partial ratio #10
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the addition.
I made a preliminary review with some things I noticed. I believe this should cover most of it, but this will certainly require another look.
The Args interface requires a bit of design work to properly work for partial_ratio_alignment
. I believe I have a solution for this, but I will need to test around with this tomorrow.
src/fuzz.rs
Outdated
let mut res = if len1 <= len2 { | ||
partial_ratio_impl( | ||
s1_iter.clone(), | ||
len1, | ||
s2_iter.clone(), | ||
len2, | ||
score_cutoff, | ||
args.score_hint, | ||
) | ||
} else { | ||
partial_ratio_impl( | ||
s2_iter.clone(), | ||
len2, | ||
s1_iter.clone(), | ||
len1, | ||
score_cutoff, | ||
args.score_hint, | ||
) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the two strings are swapped for the comparison we have to swap the start and end positions to fix them up. I guess that would come down to something along the lines of:
if len1 > len2 {
let mut res = partial_ratio_alignment(s2, len2, s1, len1, args);
std::mem::swap(&mut res.src_start, &mut res.dest_start);
std::mem::swap(&mut res.src_end, &mut res.dest_end);
return res;
}
Alternatively you can construct a new ScoreAlignment and return that.
A test would make sense for this as well. I should add one to the C++ tests too.
src/fuzz.rs
Outdated
match alignment { | ||
Some(alignment) => alignment.score, | ||
None => 0.0, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not return None if a score below score_cutoff was passed.
For now this could be:
let score = match alignment {
Some(alignment) => alignment.score,
None => 0.0,
}
args.score_cutoff.score(score)
This will likely need to be changed when making the return type of partial_ratio_alignment
alignment dependent on the presence of a score_cutoff
.
Please add a test for this as well. fn issue206
might be a good place. For the other languages I managed to find ways to iterate over functions to reduce the boilerplate for some of these tests. I didn't look into ways to achieve the same in rust so far.
let s1_char_set = s1_iter | ||
.clone() | ||
.map(|c| c.hash_char()) | ||
.collect::<HashSet<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
things like the batchcomparator and the charset should be passed as argument. This is required so the same implementation can be used from PartialRatioBatchComparator
. PartialRatioBatchComparator
should get implemented.
} | ||
|
||
let s1_iter = s1.into_iter(); | ||
let s2_vec = s2.into_iter().collect::<Vec<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit unsure about this so far. In C++ we directly use random access, but that can only be done because we assume fast random access. In Rust e.g. string is utf8 and so has no fast random access.
I believe we could write this so random access isn't required. However we would still iterate the string multiple times and so maybe for those types it's still faster to just make the copy.
I would leave it like this for now, since partial_ratio
is never super fast anyway. If we were to change it we should probably benchmark it first.
assert_eq!(result.src_end, 3); | ||
assert_eq!(result.dest_start, 1); | ||
assert_eq!(result.dest_end, 4); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are still a couple of other test in https://github.com/rapidfuzz/RapidFuzz/blob/main/tests/test_fuzz.py and https://github.com/rapidfuzz/rapidfuzz-cpp/blob/main/test/tests-fuzz.cpp that we could port over, but I didn't check this in depth so far.
Thanks for the comments! I've already taken a stab at improving typing and did the simplifications you suggested. Tests still to be extended. |
/** | ||
implementation of partial_ratio for needles <= 64. assumes s1 is already the | ||
shorter string | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It assumes len(s1) <= len(s2) not len(s1) < len(s2). I fixed the comment in the Python version as well.
The implementation should be based on the C++ implementation you can fine here: https://github.com/rapidfuzz/rapidfuzz-cpp/blob/c6a3ac87c42ddf52f502dc3ed7001c8c2cefb900/rapidfuzz/fuzz_impl.hpp#L68
The Python implementation only skips windows if they can't exist since a character is unique. The C++ implementation makes use of the fact that a score can only change by a certain distance per shift of the window to skip windows. An example would be:
"aaa" <-> "bcdef"
Here the alignments with the full length are:
- "aaa" <-> "bcd"
- "aaa" <-> "cde"
- "aaa" <-> "def"
Now if we first calculate the distance for the alignments 1 and 3 both of the have a indel distance of 6. That way we know that alignment 2 can at best have an indel distance of 4.
Looking at the C++ implementation I feel like the actual min_score
calculation in there is incorrect.. I believe instead of:
/* half of the cells that are not needed for known_edits can lead to a better score */
ptrdiff_t min_score =
static_cast<ptrdiff_t>(std::min(scores[window.first], scores[window.second])) -
static_cast<ptrdiff_t>(cell_diff + known_edits / 2);
this should be
/* half of the cells that are not needed for known_edits can lead to a better score */
size_t max_score_improvement = (cell_diff - known_edits / 2) / 2 * 2;
ptrdiff_t min_score =
static_cast<ptrdiff_t>(std::min(scores[window.first], scores[window.second])) -
static_cast<ptrdiff_t>(max_score_improvement);
The current implementation doesn't lead to incorrect results but allows skipping less cells than we really could.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved the calculation of min_score
in rapidfuzz-cpp to skip more alignments
Adds
ScoreAlignment
structpartial_ratio
partial_ratio_alignment
partial_ratio_impl
(short needle)Plus tests.