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

Provide simpler syntax to specify policy reforms and assumption updates #2292

Merged
merged 31 commits into from Apr 15, 2019

Conversation

Projects
None yet
2 participants
@martinholmer
Copy link
Collaborator

commented Apr 8, 2019

This pull request resolves issue #2291 by providing Tax-Calculator users with a less complicated syntax with which to specify policy reforms or updates to assumption parameters.

This pull request is the culmination of a series of enhancements to the Parameters class in recent pull requests #2281, #2282, #2283, #2284, #2285, #2289, and #2290. Only this #2292 pull request includes changes that make new versions of Tax-Calculator incompatible with earlier versions of Tax-Calculator. One of the major changes is the one discussed in issue #2291: changing the format of Python dictionaries supplied to the Policy.implement_reform, Consumption.update_consumption, and GrowDiff.update_growdiff methods so that the new param : year : value format is the same as used in JSON reform/assumption files. If you are specifying reforms with dictionaries (rather than in JSON files), you will have to edit the format of the dictionaries so that the old year : param : value format is transformed into the new param : year : value format. See the new Tax-Calculator/new_json.py file for a script that may help you change your personal JSON reform files.

Taken together, all these enhancements to the Parameters class mean that any other model that is already importing the Tax-Calculator package can automatically use this new syntax by creating its own classes derived from the Tax-Calculator Parameters class. An example, of the resulting simplification can be seen in Business-Taxation pull request 88 that specifies a Business-Taxation Policy class derived from the Tax-Calculator Parameters class in just a few lines of code.

This pull request also brings the name of the CPI offset policy parameter into line with other parameter names by changing it from cpi_offset to CPI_offset. A new parameter name-change capability will notify users who forget to use the new name.

The few lines of untested code cannot be tested until this pull request is merged. When it is merged, the @pytest.mark.skip decorators will be removed and those lines of code will be tested.

@codecov

This comment has been minimized.

Copy link

commented Apr 8, 2019

Codecov Report

Merging #2292 into master will decrease coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2292      +/-   ##
==========================================
- Coverage     100%   99.78%   -0.22%     
==========================================
  Files          12       12              
  Lines        2851     2839      -12     
==========================================
- Hits         2851     2833      -18     
- Misses          0        6       +6
Impacted Files Coverage Δ
taxcalc/calculator.py 99.06% <100%> (-0.94%) ⬇️
taxcalc/consumption.py 100% <100%> (ø) ⬆️
taxcalc/growdiff.py 100% <100%> (ø) ⬆️
taxcalc/parameters.py 100% <100%> (ø) ⬆️
taxcalc/policy.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed6413c...257d9d4. Read the comment docs.

@martinholmer

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2019

There has been a substantial discussion of issue #2291, and now that conversation has widened to include the issue of whether or not to make Python reform dictionaries and JSON reform files have the same parameter, year, value order, and if so, which ordering should become the standard.

I agree that this is an important topic, but I think it needs more discussion. Also, this pull request (#2292) is already quite large. So, I propose that #2292 be merged and the additional issue still being discussed can be handled in a follow-up pull request. Does that make sense?

@MattHJensen @andersonfrailey @hdoupe @codykallen @feenberg

@martinholmer martinholmer added the WIP label Apr 9, 2019

@martinholmer

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2019

I said yesterday:

I propose that #2292 be merged and the [parameter-year-value ordering] issue still being discussed can be handled in a follow-up pull request.

Actually, I think we have quickly reached a consensus on the ordering issue, so I'm going to implement the changes to the structure of Python reform/revision dictionaries in this pull request.

"col_var": "",
"col_label": [],
"vi_name": "",
"vi_vals": [],

This comment has been minimized.

Copy link
@hdoupe

hdoupe Apr 11, 2019

Collaborator

@martinholmer, what does "vi" stand for?

This comment has been minimized.

Copy link
@martinholmer

martinholmer Apr 11, 2019

Author Collaborator

vi stands for vector index.

This comment has been minimized.

Copy link
@hdoupe

hdoupe Apr 11, 2019

Collaborator

Ah, I see. Thanks.

@martinholmer martinholmer added ready and removed WIP labels Apr 11, 2019

@hdoupe

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

@martinholmer would you mind leaving #2292 open through Monday so that people have a chance to test it out?

@@ -133,7 +133,7 @@
"indexable": true,
"indexed": false,
"vi_name": "MARS",
"vi_vals": ["single", "joint", "separate", "headhousehold", "widow"],
"vi_vals": ["single", "mjoint", "marsep", "headhh", "widow"],

This comment has been minimized.

Copy link
@hdoupe

hdoupe Apr 12, 2019

Collaborator

@martinholmer These names are displayed over the parameter boxes on the webapp:
Screen Shot 2019-04-12 at 11 38 54 AM

What do you think about choosing names that are more human readable like:

["single", "married joint", "married separate", "head household", "widow"]

The difference is only a few characters, and they are readable to those who are not familiar with the naming scheme.

This comment has been minimized.

Copy link
@martinholmer

martinholmer Apr 12, 2019

Author Collaborator

I think the new values for the vector indexes are more succinct and perfectly understandable to those using Tax-Calculator (or Tax-Brain) via Python programming or the tc command-line tool. If you think users of the TaxBrain webapp need longer labels on the boxes, then by all means you should supply them with longer box labels. That's your call.

@martinholmer

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

@hdoupe asked:

would you mind leaving #2292 open through Monday so that people have a chance to test it out?

OK.

@hdoupe

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

@martinholmer Policy.implement_reform does not accept years that are strings:

import taxcalc
import json

# 2.0 styled reform
ref = {
    "policy": {
        "STD": {"2018": [10001, 10000, 10000, 10000, 10000]},
        "STD-indexed": {"2018": False},
    }
}
ref_json = json.dumps(ref)

# Use read_json_param_objects to parse JSON reform
pol = taxcalc.Calculator.read_json_param_objects(ref_json, None)

# Note that keys are integers now.
print("parsed params: ", pol["policy"])

print("Parsed parameters load without errors.")
taxcalc.Policy().implement_reform(pol["policy"])

print("Parameters with string years throw an error.")
taxcalc.Policy().implement_reform(ref["policy"])

outputs:

(taxcalc-dev) HDoupe-MacBook-Pro:Tax-Calculator henrydoupe$ python test.py 
parsed params:  {'STD': {2018: [10001, 10000, 10000, 10000, 10000]}, 'STD-indexed': {2018: False}}
Parsed parameters load without errors.
Parameters with string years throw an error.
Traceback (most recent call last):
  File "test.py", line 23, in <module>
    taxcalc.Policy().implement_reform(ref["policy"])
  File "/Users/henrydoupe/Documents/Tax-Calculator/taxcalc/policy.py", line 108, in implement_reform
    self._update(reform, print_warnings, raise_errors)
  File "/Users/henrydoupe/Documents/Tax-Calculator/taxcalc/parameters.py", line 292, in _update
    raise ValueError(msg.format(name, year))
ValueError: ERROR: KEY STD YEAR 2018 is not an integer year
(taxcalc-dev) HDoupe-MacBook-Pro:Tax-Calculator henrydoupe$ 

It seems like implement_reform should accept years that are integers or strings. Users may not understand why the JSON files require the years to be strings, but the implement_reform method requires them to be integers. This would also make it easier to serialize and deserialize reform parameters that are accepted by implement_reform.

@martinholmer

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 14, 2019

@hdoupe reported in PR #2292 that:

Policy.implement_reform does not accept years that are strings

Yes, and that is by design. Thanks for calling attention to the need to improve various method docstrings to clarify how things work. I've improved the docstrings in commit 75deaa3.

@hdoupe

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

Ok, I see. Glad my question was helpful.

@martinholmer martinholmer merged commit 4e30417 into PSLmodels:master Apr 15, 2019

4 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project Absolute coverage decreased by -0.21% but relative coverage remained the same compared to ed6413c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@martinholmer martinholmer deleted the martinholmer:better-params branch Apr 15, 2019

@martinholmer martinholmer removed the ready label Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.