-
-
Notifications
You must be signed in to change notification settings - Fork 37
Fix to default_parameters.json
#141
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #141 +/- ##
=======================================
Coverage 45.69% 45.69%
=======================================
Files 18 18
Lines 1416 1416
=======================================
Hits 647 647
Misses 769 769
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@rickecon this PR is ready for your review. |
|
@jdebacker. The file |
|
@rickecon asks:
I think it makes sense to keep the JSON files small so they are more readable. So I recommend the workflow of adding one on top of another. It adds only one line of code to layer updates like this and it saves about 10MB of disk space to use the smaller 2 sector JSON file. Both would already be included with the package so if one has OG-USA installed, they can load both if they wish. |
|
@jdebacker. I noticed that we don't have an example of this multi-country preferred parameter update workflow, neither in the OG-USA repo nor the OG-Core repo. |
|
@jdebacker. I just submitted a PR to your |
|
Looks good to me. All questions answered. Merging now. @jdebacker |
With the updates to
ogusa_default_parameters.jsonin PR #140, thetax_func_typewas not updated to "HSV". This PR makes that change.The
ogusa_default_parameters_2sector.jsonis also simplified, keeping just the parameters that need to be changed on top of theogusa_default_parameters.jsonchanges used in the one-sector model.