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

person cluster patient assignment endpoints #172

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

Conversation

ericbuckley
Copy link
Collaborator

@ericbuckley ericbuckley commented Jan 16, 2025

Description

Two new person API endpoints for assigning patients to different person clusters.

Related Issues

closes #158

Additional Notes

  • Deprecating existing create_person and update_person routes in the patient router
  • Created create_person and update_person in the person router to replace ones in patient router
  • Updated two mpi_service functions to accept a list of patients rather than just 1
  • regrouping all patient, person and seeding api endpoints into "mpi" tag in api docs

<--------------------- 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 added the feature New feature or request label Jan 16, 2025
@ericbuckley ericbuckley self-assigned this Jan 16, 2025
@ericbuckley ericbuckley marked this pull request as ready for review January 16, 2025 14:46
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.62%. Comparing base (9bd3778) to head (6946c33).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   97.56%   97.62%   +0.05%     
==========================================
  Files          31       32       +1     
  Lines        1521     1556      +35     
==========================================
+ Hits         1484     1519      +35     
  Misses         37       37              

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

@ericbuckley ericbuckley requested review from bamader and removed request for alhayward and cbrinson-rise8 January 22, 2025 18:14
Copy link
Collaborator

@bamader bamader left a comment

Choose a reason for hiding this comment

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

The code changes themselves all look fine to me. I'm wondering what the rationale is for making the functions that previously operated on a single patient or person now perform bulk retrievals / updates. That seems to actually be a fairly dangerous process (see my example above about how that could corrupt linkage results and degrade future cluster quality) if we're performing this on any kind of updates to person clusters in between processing new in coming records. This seems like a case where I'm missing some context though, such as maybe this is only used for seeding.

Comment on lines -193 to +195
def get_patient_by_reference_id(
session: orm.Session, reference_id: uuid.UUID
) -> models.Patient | None:
def get_patients_by_reference_ids(
session: orm.Session, *reference_ids: uuid.UUID
) -> list[models.Patient | None]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing wrong in the code here, just curiosity: what's the impetus for changing this from a singular patient operating function to a plural patients? Just thinking about it pedagogically in the abstract, a single patient makes sense to me because we'll be operating on single incoming records, but I think this is an area where missing context is probably in play.

Comment on lines -215 to +220
patient: models.Patient,
patients: typing.Sequence[models.Patient],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here as above, when would we be bulk updating a person cluster than adding one patient at a time? This is particularly true because that could actually change performance; e.g., adding one patient, then one patient, then one patient leaves room for the linking algorithm to actually return different match results for each subsequent addition, vs bulk adding three new patients all in one cluster update runs the risk of not properly using those second and third patients to get the most accurate linkage results. It's possible one or more might be found to be non-matches for the newly updated cluster, after adding the first patient in.

Update the Person linked on the Patient.
"""
patient = service.get_patient_by_reference_id(session, patient_reference_id)
patient = service.get_patients_by_reference_ids(session, patient_reference_id)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're just pulling one patient out of the returned bunch, that would seem to support having a function that does a single patient operation as the correct signature, right?

@@ -76,9 +86,9 @@ def delete_patient(
"""
Delete a Patient from the mpi database.
"""
patient = service.get_patient_by_reference_id(session, patient_reference_id)
patient = service.get_patients_by_reference_ids(session, patient_reference_id)[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework patient assignment API endpoints [MPIAPI]
2 participants