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 (Tax change)/(Pre-reform after-tax income) to distributional tables #1375

Merged
merged 2 commits into from
May 24, 2017

Conversation

andersonfrailey
Copy link
Collaborator

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 simply expanded_income - combined.

@MattHJensen @martinholmer

@MattHJensen
Copy link
Contributor

@andersonfrailey, this looks good other than the test error. Could you ping me again after that is resolved?

@andersonfrailey
Copy link
Collaborator Author

@MattHJensen fixed.

@MattHJensen
Copy link
Contributor

Thanks @andersonfrailey. Merging. Could you add this contribution to #1364?

@martinholmer
Copy link
Collaborator

@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 create_difference_table utility function is called aftertax_perc. That name strongly suggests to me that it measures the percentage change in after-tax income. But maybe that's just me getting confused.

But three months later you said (my emphasis) in pending pull request #1521:

In PR #1375, I added a feature to include the percent change in after-tax income in the difference table.

So, did you also get confused about what #1375 really did? Did you get confused because you interpreted aftertax_perc to mean the percentage change in after-tax income? If so, you should change the name of the new statistic to describe exactly what the statistic measures.

Also, I wonder when looking over the changes in #1375, why you didn't compute the new statistic in the means_and_comparisons utility function along with all the other statistics in the difference table. Forgetting that you did not do that is what is generating all the unit test errors I mentioned in #1521.

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.

@MattHJensen @hdoupe

@andersonfrailey
Copy link
Collaborator Author

@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.

Also, I wonder when looking over the changes in #1375, why you didn't compute the new statistic in the means_and_comparisons utility function along with all the other statistics in the difference table. Forgetting that you did not do that is what is generating all the unit test errors I mentioned in #1521.

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.

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.

4 participants