-
Notifications
You must be signed in to change notification settings - Fork 35
Update simulation (for app v2) to store output for household calculations #2827
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2827 +/- ##
==========================================
- Coverage 77.03% 75.19% -1.85%
==========================================
Files 53 53
Lines 1903 1955 +52
Branches 243 252 +9
==========================================
+ Hits 1466 1470 +4
- Misses 387 435 +48
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
079d356
to
0422cdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this Sakshi! A couple nits; technically, if you don't want to add them and just want to merge, also feel free.
policy_id INT NOT NULL | ||
policy_id INT NOT NULL, | ||
-- output_json stores calculation results for household simulations only | ||
-- For geography simulations, outputs are stored in report_outputs table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can you say that it's for single-sim households (comparisons are still stored in report_outputs) and that for geo sims, it's because we don't have the API capability to calculate them separately?
policy_id INT NOT NULL, | ||
-- output_json stores calculation results for household simulations only | ||
-- For geography simulations, outputs are stored in report_outputs table | ||
output_json JSON DEFAULT NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Same as for initialise.sql
) | ||
|
||
if not success: | ||
raise BadRequest("No fields to update") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, non-blocking: I think the message is wrong here maybe?
""" | ||
print(f"Updating simulation {simulation_id} with output") | ||
# Automatically update api_version on every update to latest | ||
api_version: str = COUNTRY_PACKAGE_VERSIONS.get(country_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accolade: Good catch, we do need this
Fixes PolicyEngine/policyengine-app-v2#226
This PR
output_json
column tosimulations
table.PATCH
endpoint to store household calculation outputs for each simulation.Note: The production database schema has been modified using