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

Handle threshold_match_probablity 0 in predict() #2420 #2425

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

browo097302
Copy link
Contributor

Updated predict_from_comparison_vectors_sqls_using_settings function to handle threshold_match_probability 0 as None. Modified the logic in lines 100-117, this is to prevent maths errors when doing log2.

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

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

Existing Issue #2420

Give a brief description for the solution you have provided

Whenever this predict function is called it will first check for 0 and assign threshold_match_probability to None.
Then when it is later passed into log2 it won't return a math error.
As its a minor change I haven't changed documentation or changelog.

PR Checklist

  • Added documentation for changes
  • Added feature to example notebooks or tutorial (if appropriate)
  • Added tests (if appropriate)
  • Updated CHANGELOG.md (if appropriate)
  • Made changes based off the latest version of Splink
  • Run the linter
  • Run the spellchecker (if appropriate)

…tion

Updated function to handle threshold_match_probability 0 as None. 
Modified the logic in lines 100-117, this is to prevent maths errors when doing log2.
@browo097302 browo097302 marked this pull request as ready for review September 26, 2024 12:58
@@ -97,7 +97,9 @@ def predict_from_comparison_vectors_sqls(
bf_terms,
sql_infinity_expression,
)

# Add condition to treat case of 0 as None
if threshold_match_probability == 0:
Copy link
Member

Choose a reason for hiding this comment

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

There's whitespace at the end of these lines at the moment hence ruff is failing.

Really the best solution is to process using ruff, but removing the whitespace manually should do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, yeah sorry i thought I'd fixed that let me have another look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should have done it now, it was on line 102, sorry im very new to this stuff.

@RobinL RobinL merged commit 88376ac into moj-analytical-services:master Sep 30, 2024
25 checks passed
@RobinL
Copy link
Member

RobinL commented Sep 30, 2024

Thanks. Closes #2420

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