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

Modify how taxcalc handles CPI index toggling #2364

Merged
merged 3 commits into from Aug 6, 2019

Conversation

@andersonfrailey
Copy link
Contributor

commented Jul 24, 2019

This PR addressed issue #2363. I've slightly modified how taxcalc handles reform parameters ending in -indexed to avoid getting an error like the one defined in the linked issue. I also added a test for this type of reform. I'm happy to update this PR if @martinholmer or @MattHJensen decided on a different method of fixing the bug.

cc @hdoupe

@codecov

This comment has been minimized.

Copy link

commented Jul 24, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2364   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          13      13           
  Lines        2745    2747    +2     
======================================
+ Hits         2745    2747    +2
Impacted Files Coverage Δ
taxcalc/parameters.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 6551405...aaca9d9. Read the comment docs.

@andersonfrailey andersonfrailey changed the title Indexing Modify how taxcalc handles CPI index toggling Jul 24, 2019

@andersonfrailey andersonfrailey referenced this pull request Jul 24, 2019
@MattHJensen

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Thanks very much, @andersonfrailey.

@martinholmer

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

@andersonfrailey, Thanks for the bug report in issue #2363 and the proposed code change in #2364 .

This bug remained undetected for a long time because the pytest suite did not include a reform that changed the value of CPI_offset in the same year that the indexing status of a tax policy variable was being changed. Your code change and the new test you added demonstrate that changing CPI_offset in the same year as a policy variable's indexing status is changed no longer causes Tax-Calculator to crash. That's a big improvement. But the new test should be stronger: we would like some assurance that your proposed code change not only avoids a crash, but also produces the correct result.

Can you revise the new test to do that?

One way to do this is to simulate two reforms:

reform1 = {'CTC_c-indexed': {2020: True}}
offset = -0.005
reform2 = {'CTC_c-indexed': {2020: True}, 'CPI_offset': {2020: offset}}

Use reform1 to create a policy1 object and reform2 to create a policy2 object. Then extract the 2019, 2020, and 2021 values of CTC_c from the policy1 object, and use them and the value of offset to calculate the expected values of CTC_c for those three years under reform2. Next extract the actual 2019, 2020, and 2021 values of CTC_c from the policy2 object. And finally, assert that the three actual values are equal to the three expected values using np.allclose.

Does this make sense? If not, please ask questions. It would be nice to strengthen the test and merge this pull request quickly, so that we can issue, in the next few days, a new version of Tax-Calculator that fixes this bug.

@andersonfrailey

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@martinholmer yes, that all makes sense. I'll jump on that.

@martinholmer martinholmer merged commit 0c5f2f0 into PSLmodels:master Aug 6, 2019

4 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 6551405
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.