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 _cpi_offset parameter and associated logic and tests #1667

Merged
merged 4 commits into from
Nov 17, 2017
Merged

Add _cpi_offset parameter and associated logic and tests #1667

merged 4 commits into from
Nov 17, 2017

Conversation

martinholmer
Copy link
Collaborator

@martinholmer martinholmer commented Nov 15, 2017

This pull request attempts to add the CPI-offset capability discussed in issue #1621.

The required changes in current_law_policy.json have been made except for the section titles. @MattHJensen, presumably this parameter will be available to TaxBrain users, so I'd like to hear from you what the section_1 and section_2 titles should be, and also where on the TaxBrain input page you intend to show this new parameter (presumably at the very top, right?).

Any suggestions or comments are welcome.

@Amy-Xu @andersonfrailey @hdoupe @GoFroggyRun @codykallen @evtedeschi3

@codecov-io
Copy link

codecov-io commented Nov 15, 2017

Codecov Report

Merging #1667 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1667   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        2856    2880   +24     
======================================
+ Hits         2856    2880   +24
Impacted Files Coverage Δ
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 c36b449...22c79f8. Read the comment docs.

@martinholmer martinholmer changed the title [WIP] Add _cpi_offset parameter and associated logic and test Add _cpi_offset parameter and associated logic and test Nov 16, 2017
@MattHJensen
Copy link
Contributor

@martinholmer, what do you think about adding a test that checks that a reform with an _ACPIU growdiff response produces the same result as a reform cpi offset?

"growdiff_response": {
"_ACPIU": {"2017": [-0.0025]}

@MattHJensen
Copy link
Contributor

@MattHJensen, presumably this parameter will be available to TaxBrain users, so I'd like to hear from you what the section_1 and section_2 titles should be, and also where on the TaxBrain input page you intend to show this new parameter (presumably at the very top, right?).

Yes, top of page makes sense to me.

How about:

_section_1: Parameter Indexing
_section_2: Offsets

@martinholmer martinholmer changed the title Add _cpi_offset parameter and associated logic and test Add _cpi_offset parameter and associated logic and tests Nov 17, 2017
@martinholmer
Copy link
Collaborator Author

@MattHJensen asked:

what do you think about adding a test that checks that a reform with an _ACPIU growdiff response produces the same result as a reform cpi offset?

"growdiff_response": {"_ACPIU": {"2017": [-0.0025]}

Thanks for the good suggestion. The test has been added.

@MattHJensen
Copy link
Contributor

LGTM. Thanks @martinholmer!

@martinholmer
Copy link
Collaborator Author

Does anybody else want to review the code and tests in #1667 that add a new _cpi_offset policy parameter?
@MattHJensen seems to be in a hurry to create release 0.13.2

Speaking of 0.13.2, why do we have multiple House TCJA reform files in taxcalc/reforms when the House has passed a bill. Don't we want one TCJA_House.json file that represents the best Tax-Calculator can do at modeling the House-passed bill? Presumably that would include use of the new _cpi_offset parameter (see pending pull request #1667) and a new parameter to implement the logic discussed in issue #1671.

@MattHJensen @codykallen
@Amy-Xu @andersonfrailey @hdoupe @GoFroggyRun

@codykallen
Copy link
Contributor

@martinholmer asked

Speaking of 0.13.2, why do we have multiple House TCJA reform files in taxcalc/reforms when the House has passed a bill. Don't we want one TCJA_House.json file that represents the best Tax-Calculator can do at modeling the House-passed bill?

We made multiple versions to account for how the bill evolved. This allowed the comparison of multiple versions, although this is less useful now that it has been passed by the House.

@martinholmer
Copy link
Collaborator Author

@codykallen said:

We made multiple versions to account for how the bill evolved. This allowed the comparison of multiple versions ...

I think that approach was very sensible.

although this is less useful now that it has been passed by the House.

That's my point. Going forward the evolution of the House bill is pretty academic.
Why not focus on the evolution of the Senate bill and have one JSON reform file for the House-passed bill?

@MattHJensen

@MattHJensen
Copy link
Contributor

That's my point. Going forward the evolution of the House bill is pretty academic.
Why not focus on the evolution of the Senate bill and have one JSON reform file for the House-passed bill?

I'd prefer to keep these because some of our users have wanted to compare different versions of the bill and see how priorities shifted as it evolved over time. I also don't see much a maintenance cost to keeping them. We will need a better and more-consistent naming scheme though.

@MattHJensen
Copy link
Contributor

would include use of the new _cpi_offset parameter (see pending pull request #1667) and a new parameter to implement the logic discussed in issue #1671.

I agree that the json files should be updated to include the new _cpi_offset parameter. I'm not sure about #1671 -- the legislative text really is ambiguous as far as I can tell. @codykallen, what do you think?

@codykallen
Copy link
Contributor

@MattHJensen, there certainly is at least some ambiguity. Perhaps we should open a separate issue to discuss any potential notes and comments to add to each of the TCJA json files.

@MattHJensen
Copy link
Contributor

Merging. Thanks again @martinholmer

cc @GoFroggyRun

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.

5 participants