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 Expanded lookup table endpoint #3674

Merged

Conversation

tyler-spangler6
Copy link
Contributor

What was the problem?

This implements a new classification lookup table to classify Veteran contentions. The research for this new method is in Free Text Classification Research.

This establishes the new method and adds the endpoint. This will not be implemented in vets-api until we are able to conduct testing on the processing time and load testing on the API to ensure that it does not slow down the full submit process for the 526EZ.

Associated tickets or Slack threads:

How does this fix it?1

How to test this PR

  • From cc-app directory run pytest

Footnotes

  1. Pull-Requests guidelines. If PR is significant, update Current Software State wiki page.

Copy link
Contributor

github-actions bot commented Oct 30, 2024

Test Results

136 tests  ±0   136 ✅ ±0   25s ⏱️ ±0s
 39 suites ±0     0 💤 ±0 
 39 files   ±0     0 ❌ ±0 

Results for commit 189207e. ± Comparison against base commit fee6499.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Oct 30, 2024

JaCoCo Test Coverage

Overall Project 73%

There is no coverage information present for the Files changed

@tyler-spangler6 tyler-spangler6 marked this pull request as ready for review October 30, 2024 20:21
@tyler-spangler6 tyler-spangler6 requested a review from a team as a code owner October 30, 2024 20:21
Copy link

@williamphelps13 williamphelps13 left a comment

Choose a reason for hiding this comment

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

Awesome job! Nice thorough testing!

Comment on lines +253 to +254
classification_code = classification["classification_code"]
classification_name = classification["classification_name"]

Choose a reason for hiding this comment

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

Would this have any benefit here (and in the next conditional)?

Suggested change
classification_code = classification["classification_code"]
classification_name = classification["classification_name"]
classification_code = classification.get("classification_code")
classification_name = classification.get("classification_name")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using dict[] is preferred if we are certain the key exists which we are since we have the defaults set in the lookup_tables.get() methods. I think the advantage of using dict.get() is that handles the keyerror using the default. I am good to switch it if it helps readability and consistency though.

@tyler-spangler6 tyler-spangler6 merged commit 180f14e into develop Oct 31, 2024
1 check passed
@tyler-spangler6 tyler-spangler6 deleted the conditions-626-expanded-classification-endpoint branch October 31, 2024 18: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