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 distance_threshold check #1718

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Conversation

ThomasHepworth
Copy link
Contributor

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

Is your Pull Request linked to an existing Issue or Pull Request?

#1717

Give a brief description for the solution you have provided

Adds a quick validation check to the distance_threshold value, erroring out should an invalid threshold be entered by the user.

Error demo
import splink.comparison_level_library as cll

# Passes
cll.JaroWinklerLevel("test", 0.8).get_comparison_level("duckdb")

# Errors
cll.JaroWinklerLevel("test", 1.2)

I'm happy to close this if you'd rather not add this in, or, would rather add it in at a later date.

Note - we may want to change the function to only check values between 0 and 1 (remove the upper and lower bound args), making the code easier to read, but reducing flexibility. However, given our current suite of distance functions, I believe this is fine (the lev function family really only wants to check that n > 0).

PR Checklist

Copy link
Member

@RobinL RobinL left a comment

Choose a reason for hiding this comment

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

Love it

Base automatically changed from jaro_winkler_level to splink4_dev November 10, 2023 13:46
@ThomasHepworth ThomasHepworth merged commit 30ad12f into splink4_dev Nov 10, 2023
1 of 6 checks passed
@ThomasHepworth ThomasHepworth deleted the validate_distance_thresholds branch November 10, 2023 13:47
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