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

remove kwargs #249

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

remove kwargs #249

wants to merge 27 commits into from

Conversation

ericbuckley
Copy link
Collaborator

@ericbuckley ericbuckley commented Mar 13, 2025

Description

Removing kwargs from Algorithm, in favor of explicit definitions of the necessary fields. Additionally, a large refactor of the Algorithm parameter sent to linkage, this is now an instance schema.Algorithm not model.Algorithm.

Related Issues

closes #223

Additional Notes

  • Evaluation Context a new section in the Algorithm configuration was created for capturing context parameters. This is very similar to how the existing kwargs functioned, but a) adds structure b) is defined once per Algorithm rather than once per pass.
  • Removing rule function since the removal of dibbs-basic, there is only one evaluation rule. It's been removed from the configuration, since it can no longer be configured.
  • Making evaluator function name easier the inclusion of the "func:..." syntax was to allow for some flexibility in testing the phdi code against RecordLinker. That flexibility is no longer needed, and using an enum name rather than a function path string seems easier for users to grasp. (ie, users should have to know or care where the function lives in the codebase).
  • Swap models.Algorithm for schemas.Algorithm Theres a one way relationship between schemas and models, the former knows about the latter, but not the other way around (necessary to prevent circular imports). This has created some problems when passing models.Algorithm around in the linkage code, for instance we always have to convert a blocking key id/string to a BlockingKey object for evaluation, same goes for Feature. The schemas class has everything we need for parsing the data into the appropriate objects for evaluation, it made a lot of sense and reduced some of the logic needed in recordlinker/linking by making this switch.
  • Removing models.AlgorithmPass. Some of the validation logic was being duplicated in the schemas.Algorithm and models.Algorithm, additionally, any time we use the Algorithm, we always want to take a look at the passes. We're already using JSON heavily to store elements of the configuration, so just leaning more into this strategy for storing the passes.

<--------------------- REMOVE THE LINES BELOW BEFORE MERGING --------------------->

Checklist

Please review and complete the following checklist before submitting your pull request:

  • I have ensured that the pull request is of a manageable size, allowing it to be reviewed within a single session.
  • I have reviewed my changes to ensure they are clear, concise, and well-documented.
  • I have updated the documentation, if applicable.
  • I have added or updated test cases to cover my changes, if applicable.
  • I have minimized the number of reviewers to include only those essential for the review.

Checklist for Reviewers

Please review and complete the following checklist during the review process:

  • The code follows best practices and conventions.
  • The changes implement the desired functionality or fix the reported issue.
  • The tests cover the new changes and pass successfully.
  • Any potential edge cases or error scenarios have been considered.

@ericbuckley ericbuckley self-assigned this Mar 13, 2025
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.11%. Comparing base (f6cc2d2) to head (37b535d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
+ Coverage   97.80%   98.11%   +0.30%     
==========================================
  Files          33       32       -1     
  Lines        1731     1697      -34     
==========================================
- Hits         1693     1665      -28     
+ Misses         38       32       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ericbuckley ericbuckley marked this pull request as ready for review March 18, 2025 19:00
@ericbuckley ericbuckley requested a review from bamader as a code owner March 18, 2025 19:00
@ericbuckley ericbuckley requested a review from m-goggins as a code owner March 18, 2025 19:00
@ericbuckley ericbuckley changed the title Feature/223 remove kwargs remove kwargs Mar 19, 2025
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.

remove kwargs from AlgorithmPass
1 participant