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

Api enhanced algo #656

Merged
merged 13 commits into from
Jun 13, 2023
Merged

Api enhanced algo #656

merged 13 commits into from
Jun 13, 2023

Conversation

bamader
Copy link
Collaborator

@bamader bamader commented Jun 8, 2023

Expose Enhanced Algo on Endpoint

Summary

This PR allows a user to invoke the DIBBs enhanced algorithm from a cloud endpoint with a new parameter in the input bundle.

Related Issue

Fixes #635

Additional Information

This PR is currently blocked by #655 , which makes some last SDK changes that this PR references. Once that PR lands, the requirements.txt file in this PR can be changed back to point to the main branch before merging.

@bamader bamader mentioned this pull request Jun 8, 2023
@bamader bamader changed the base branch from sdk-linkage-indexing to main June 8, 2023 20:28
@bamader
Copy link
Collaborator Author

bamader commented Jun 8, 2023

I have literally no idea what's happening with the file in the assets folder. None of that should be changed. After the referenced PR lands, that's exactly how everything in the SDK should look.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #656 (8248215) into main (7578190) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #656   +/-   ##
=======================================
  Coverage   96.37%   96.37%           
=======================================
  Files          45       45           
  Lines        2538     2539    +1     
=======================================
+ Hits         2446     2447    +1     
  Misses         92       92           
Flag Coverage Δ
unit-tests 96.37% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Copy link
Collaborator

@m-goggins m-goggins left a comment

Choose a reason for hiding this comment

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

Looks good!

One thing that occurred to me is that you added more info to the test_bundle and I'm wondering how that fits in with the changes Brady (or maybe Kenneth) made to standardizing our test assets (if at all)?

@bamader
Copy link
Collaborator Author

bamader commented Jun 13, 2023

@m-goggins I went back and did some digging on Kenneth's test standardizations, and it turns out that's for a different test generator. It's basically a helper function that loads up a list of lists array that we use in a couple of the helper function pandas tests. Whereas here, the changes I made are actually because I noticed that some of our test cases were outright missing patient info 😮 . One of the patients didn't have an MRN when they should have, and one of the patients had the same UUID as another patient for them, so it was correcting those errors that created changes in the test files here.

@bamader bamader requested a review from m-goggins June 13, 2023 12:37
@bamader bamader merged commit 3d0a729 into main Jun 13, 2023
@bamader bamader deleted the api-enhanced-algo branch June 13, 2023 21:20
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.

Make the enhanced algorithm usable in our containers
2 participants