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

Mtr e26270 #987

Merged
merged 10 commits into from
Oct 17, 2016
Merged

Mtr e26270 #987

merged 10 commits into from
Oct 17, 2016

Conversation

codykallen
Copy link
Contributor

Adds calculation of MTR on e26270 (partnership and S-corp income)

@codecov-io
Copy link

codecov-io commented Oct 14, 2016

Current coverage is 98.27% (diff: 100%)

Merging #987 into master will increase coverage by <.01%

@@             master       #987   diff @@
==========================================
  Files            35         35          
  Lines          2085       2089     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2049       2053     +4   
  Misses           36         36          
  Partials          0          0          

Powered by Codecov. Last update afd3d4b...60e298a

@codykallen
Copy link
Contributor Author

I tested this with a reform of pass-through rates:

reform_pt = { 2017: {
'_PT_rt5': [0.3],
'_PT_rt6': [0.3,
'_PT_rt7': [0.3]
}
}

I calculated average MTRs on e02000 and e26270 under the baseline and reform, weighting by the absolute value of the income type.
Baseline MTR, e02000: 32.77%
Baseline MTR, e26270: 30.60%
Reform MTR, e02000: 29.08%
Reform MTR, e26270: 25.34%

@@ -322,6 +326,8 @@ def mtr(self, variable_str='e00200p',
self.records.e00900 = seincome_var + finite_diff
elif variable_str == 'e00650':
self.records.e00600 = divincome_var + finite_diff
elif variable_str == 'e26270':
self.records.e02000 = schEincome_var + finite_diff
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codykallen Why add finite_diff to e02000 instead of e26270 here? Seems like adding to e26270 is what would help this MTR enter functions.SchXYZ() in a way that accounts for potential differences between rates on ordinary income and pass-through income.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finite_diff is already being added to e26270 in the line:
setattr(self.records, variable_str, variable + finite_diff)

However, e26270 is included in e02000, so you have to increase e02000 as well. If not, then you would be calculating the MTR from adding finite_diff to e26270 but substracting it from rental income.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Thanks @codykallen, LGTM then.

@jdebacker
Copy link
Member

@codykallen - thanks for opening this, it'll be helpful give the updated to pass-through income logic in taxcalc.

@martinholmer
Copy link
Collaborator

@codkallen, Your pull request #987 looks fine except for one thing: you need to add a test to prevent a decline in code coverage. Adding something like this at the end of the test_Calculator_mtr function (in the taxcalc/tests/test_calculate.py file) would be the minimal test:

    (_, _, mtr_combined) = calc.mtr(variable_str='e26270')
    assert type(mtr_combined) == np.ndarray

@MattHJensen @Amy-Xu @GoFroggyRun @andersonfrailey

@martinholmer
Copy link
Collaborator

@codykallen, Thanks for adding the test of the new mtr logic in pull request #987.

However, I don't understand why you have added the extra variables to the Records.VALID_READ_VARS set. You're adding four variables: 'FDED', 'e11550', 'e11070', 'e11580'. Why? None of these four are in the current puf.csv file.

@martinholmer martinholmer merged commit 86ee9dc into PSLmodels:master Oct 17, 2016
@martinholmer
Copy link
Collaborator

@codykallen, Thanks so much for pull request #987, which has been merged into master branch.

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.

None yet

4 participants