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

Add partial ratio #10

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add partial ratio #10

wants to merge 14 commits into from

Conversation

ljnsn
Copy link

@ljnsn ljnsn commented Dec 10, 2024

Adds

  • ScoreAlignment struct
  • partial_ratio
  • partial_ratio_alignment
  • partial_ratio_impl (short needle)

Plus tests.

Copy link
Member

@maxbachmann maxbachmann left a 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
Comment on lines 227 to 245
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,
)
};
Copy link
Member

@maxbachmann maxbachmann Dec 11, 2024

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 Show resolved Hide resolved
src/fuzz.rs Outdated Show resolved Hide resolved
src/fuzz.rs Outdated
Comment on lines 201 to 204
match alignment {
Some(alignment) => alignment.score,
None => 0.0,
}
Copy link
Member

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.

Comment on lines +314 to +317
let s1_char_set = s1_iter
.clone()
.map(|c| c.hash_char())
.collect::<HashSet<_>>();
Copy link
Member

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<_>>();
Copy link
Member

@maxbachmann maxbachmann Dec 11, 2024

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.

src/fuzz.rs Outdated Show resolved Hide resolved
assert_eq!(result.src_end, 3);
assert_eq!(result.dest_start, 1);
assert_eq!(result.dest_end, 4);
}
Copy link
Member

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.

@ljnsn
Copy link
Author

ljnsn commented Dec 12, 2024

Thanks for the comments! I've already taken a stab at improving typing and did the simplifications you suggested. Tests still to be extended.

Comment on lines +267 to +270
/**
implementation of partial_ratio for needles <= 64. assumes s1 is already the
shorter string
*/
Copy link
Member

@maxbachmann maxbachmann Dec 12, 2024

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.

Copy link
Member

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

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