-
-
Notifications
You must be signed in to change notification settings - Fork 159
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 (Tax change)/(Pre-reform after-tax income) to distributional tables #1375
Conversation
@andersonfrailey, this looks good other than the test error. Could you ping me again after that is resolved? |
@MattHJensen fixed. |
Thanks @andersonfrailey. Merging. Could you add this contribution to #1364? |
@andersonfrailey, I don't understand the changes you made three months ago in #1375. The merged pull request #1375, and issue #1371 that requested it, are clear that the new statistic was to be the change in tax liability as a percent of pre-reform after-tax income. But the new variable you added to the But three months later you said (my emphasis) in pending pull request #1521:
So, did you also get confused about what #1375 really did? Did you get confused because you interpreted Also, I wonder when looking over the changes in #1375, why you didn't compute the new statistic in the I think you need to rethink #1375 and #1521 to get a consistent approach to the new statistic you want to add to the difference table. Then implement that consistent approach in a revised pull request #1521. I would suggest doing this now rather than waiting for comments from any of the Continuum developers because otherwise you'll be waiting for a long time. If you can get #1521 ready to be merged in time to include it in version 0.9.3, then somebody will be able use version 0.9.3 to test the TaxBrain enhancements required to display the new statistic. If what you do in #1521 is not ideal support for TaxBrain, then we can change that later. |
@martinholmer PR #1521 won't be ready for 0.9.3 so no need to wait on releasing it for this. I believe my description of PR #1521 was a little misleading. PR #1375 added a column to the tables that showed (tax change)/(baseline income), as requested in #1371. PR #1521 aims to add that to the tables produced in TaxBrain.
I will review how I implemented this code in the next few days to see if there are better ways to get the same results. @hdoupe and I will be working on #1521 intermittently as well to get everything working, but again, not need to wait on releasing 0.9.3 to include this. |
This PR is in response to issue #1371. It adds a column to the differences table that shows
(tax change) / (pre-reform after tax income)
.I also added a new variable to the records class
aftertax_income
, which is simplyexpanded_income - combined
.@MattHJensen @martinholmer