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

Policy parameter update with 2018 values #2212

Merged
merged 16 commits into from
Feb 1, 2019

Conversation

Peter-Metz
Copy link
Contributor

@Peter-Metz Peter-Metz commented Jan 28, 2019

This PR uses the process described in Issue #2183 to update policy parameter values in policy_current_law.json to reflect IRS final forms for tax year 2018. After updating final 2018 parameter values, I ran ppp.py to calculate parameter values through 2026. Where the IRS forms changed in layout (e.g. Form 1040), I updated the "irs_ref" value to point users to the correct line in the 2018 forms.

The IRS forms and instructions used to make these updates can be found below:

Form 1040 and instructions, Form 1040 (Schedule 1), Form 1040 (Schedule 2), Form 1040 (Schedule 4), Form 1040 (Schedule A) and instructions

Form W-2 and instructions

Form 6251 and instructions

Form 8863 and instructions

Form 8812 and instructions

Addendum (added by @martinholmer on 2019-02-01):
Using actual 2018 parameter values as the base for indexing (rather than using the actual 2017 values as before this pull request), produces more accurate tax results for years beginning with 2018. The net results of this parameter updating are modest because many important actual 2018 parameter values were already known from the TCJA legislation. For example, when using the PUF input data the 2021 income tax liability declines from $1764.7 billion to $1764.6 billion, which is a decline of $0.1 billion or about six one-thousandths of one percent, and the 2021 payroll tax liability rises from $1322.9 billion to $1324.6 billion, which is a rise of $1.7 billion or about one-tenth of one percent.

Addendum (added by @martinholmer on 2019-02-06):
A couple of the EITC parameters updated in this pull request were close approximations of the exact values. The exact values have been added in pull request #2220.

@martinholmer
Copy link
Collaborator

@Peter-Metz, Thanks for all the work that is included in your pull request #2212. But as you can see from this

screen shot 2019-01-28 at 5 14 05 pm

that there are number of procedural problems. The biggest procedural problem is that your development work branched off from the master branch seventeen days ago just after PR#2185 was merged into the master branch. (You can see this in the Network time-line graph on GitHub.) Meanwhile there have been several important revisions to the master branch (including updating the projections in PR#2196 and updating the OASDI maximum taxable earnings parameter in PR#2203). This means, among other things, that your pull request undoes all the work I did with the ppp.py script when @andersonfrailey's new projections were incorporated into the master branch in PR#2196. The fact that your PR is based on data and code that is out-date can be seen in your personal "origin" repository (where you forked PSLmodels/Tax-Calculator):

screen shot 2019-01-28 at 5 23 33 pm

Your development branch (which is being submitted as PR#2212 for incorporation into the PSLmodels/Tax-Calculator master branch) is 7 commits ahead (those are your changes on your 2018-param development branch) and 71 commits behind the PSLmodels/Tax-Calculator master branch.

The solution is to merge the most recent version of the PSLmodels/Tax-Calculator master branch into your 2018-param development branch. If you're not certain how to do this, it would be good for you to read the documentation including the contributor guide and the documentation to which it points.

@Peter-Metz
Copy link
Contributor Author

@martinholmer, thank you for the explanation. The latest commit should fix the merge conflicts. The conflicts were results of the numbers generated by ppp.py, and did not affect any of the 2018 parameters that I entered. I'm not quite sure why the ppp.py script generated different numbers, but if you think the script needs to be re-run, I am happy to do so in a separate PR.

@martinholmer
Copy link
Collaborator

@Peter-Metz said:

I'm not quite sure why the ppp.py script generated different numbers

Because you were using the old projections as I explained earlier in this comment.

@Peter-Metz
Copy link
Contributor Author

@martinholmer

Ah, understood. Thanks.

@martinholmer
Copy link
Collaborator

martinholmer commented Jan 29, 2019

@Peter-Metz, I'm curious about why you changed some 2017 policy parameter values as part of your 2018 parameter update work in PR #2212. For example, you changed (slightly) the values for the _EITC_ps and _EITC_ps_MarriedJ parameters for 2017. Was that intentional? If so, can you provide a link to the IRS documents that show the 2017 values you switched to? Here's the first (_EITC_ps) change:

screen shot 2019-01-29 at 4 58 58 pm

And here's the second (_EITC_ps_MarriedJ) change:

screen shot 2019-01-29 at 5 01 04 pm

Also, the GitHub diff for the _II_brk6 parameter is confusing. What's going on there? Here's the diff image:

screen shot 2019-01-29 at 5 10 29 pm

@martinholmer
Copy link
Collaborator

@Peter-Metz, Now that PR #2211 has been merged into the PSLmodels / Tax-Calculator master branch, you need to do two things on your computer in the following order:

  1. On your local master branch, you need to run in the top T-C directory ./gitsync (or just gitsync if working on Windows), and

  2. On you local 2018-param branch, you need to run git merge master.

@Peter-Metz
Copy link
Contributor Author

@martinholmer:

you need to do two things on your computer in the following order:

Thanks for these instructions.

you changed (slightly) the values for the _EITC_ps and _EITC_ps_MarriedJ parameters for 2017

This was intentional, but on second thought, I'm not sure I was correct in doing this. For example, I changed 2017, _EITC_ps for 0 children from $8,340 to $8,350. The phaseout begins for incomes "at least" $8,350 but "less than" $8,400. I now see that if AGI is being calculated in increments of $10, the original value was correct. If you agree with this assessment, I will push a commit that reverts the 2017 values for _EITC_ps and _EITC_ps_MarriedJ back to their original values, and fix the 2018 values with that same logic.

Also, the GitHub diff for the _II_brk6 parameter is confusing. What's going on there?

First, I changed 2018 "widow" from $500,000 to $600,000 because I believe there was a typo. In terms of the values from 2019-2026, that looks like a difference stemming from ppp.py, although I'm not sure why that didn't cause a merge conflict. I am happy to revert the 2019-2026 values back or re-run ppp.py on my computer now that my development branch is up to date. Either way, if you agree that the 2018 value for "widow" was a typo, ppp.py will have to be re-run.

@martinholmer
Copy link
Collaborator

@Peter-Metz said in PR #2212:

you changed (slightly) the values for the _EITC_ps and _EITC_ps_MarriedJ parameters for 2017

This was intentional, but on second thought, I'm not sure I was correct in doing this. For example, I changed 2017, _EITC_ps for 0 children from $8,340 to $8,350. The phaseout begins for incomes "at least" $8,350 but "less than" $8,400. I now see that if AGI is being calculated in increments of $10, the original value was correct. If you agree with this assessment, I will push a commit that reverts the 2017 values for _EITC_ps and _EITC_ps_MarriedJ back to their original values, and fix the 2018 values with that same logic.

I agree, so please do these things with the EITC parameters.

Also, the GitHub diff for the _II_brk6 parameter is confusing. What's going on there?

First, I changed 2018 "widow" from $500,000 to $600,000 because I believe there was a typo. In terms of the values from 2019-2026, that looks like a difference stemming from ppp.py, although I'm not sure why that didn't cause a merge conflict. I am happy to revert the 2019-2026 values back or re-run ppp.py on my computer now that my development branch is up to date. Either way, if you agree that the 2018 value for "widow" was a typo, ppp.py will have to be re-run.

Which of the IRS forms for which you provide links shows the widow amount at $600,000?

Assuming you have caught a mistake (good work!), the pull request values for _II_brk6 for 2018+ seem just fine. No need to do anything except verify on the forms that $600,000 is the correct number for widow.

@martinholmer
Copy link
Collaborator

@Peter-Metz, After fixing the issues discussed in this comment, you need to do one more thing before addressing the test failures. Change in policy.py the value of LAST_KNOWN_YEAR from 2017 to 2018. Then rerun the tests on your computer. If you're puzzled about how to handle any of the test failures, please ask by posting a comment in this PR conversation. This PR is shaping up very nicely. Thanks for the major contribution.

@martinholmer
Copy link
Collaborator

martinholmer commented Jan 31, 2019

@martinholmer said incorrectly in #2212:

Which of the IRS forms for which you provide links shows the widow amount at $600,000?

Assuming you have caught a mistake (good work!), the pull request values for _II_brk6 for 2018+ seem just fine. No need to do anything except verify on the forms that $600,000 is the correct number for widow.

Actually, the values currently in #2212 for _II_brk6 for 2019+ are not fine. They are apparently based on the use of the old projection factors earlier in the #2212 work. After correcting the 2018 value for widow there should be differences (relative to the master branch) for only the widow values. But that not the case now as shown in the screen shot below (where the master branch is on the left and the development branch is on the right):

screen shot 2019-01-31 at 10 40 25 am

What we want in this comparison is the 500000-to-600000 change for widow in 2018, and changes in 2019+ only for the widow value.

@Peter-Metz

@Peter-Metz
Copy link
Contributor Author

@martinholmer:

Which of the IRS forms for which you provide links shows the widow amount at $600,000?

This can be found in the 1040 Instructions, Schedule Y-1, page 113.

Change in policy.py the value of LAST_KNOWN_YEAR from 2017 to 2018.

Will do.

Actually, the values currently in #2212 for _II_brk6 for 2019+ are not fine.

Thanks, I came to the same conclusion while running the tests and have since fixed this locally by re-running ppp.py.

If you're puzzled about how to handle any of the test failures, please ask by posting a comment in this PR conversation.

I have managed to make the necessary changes to pass most of the tests, but would appreciate some guidance on one failure in particular. The PR fails the function test_json_file_contents in the test_parameters.py file because some of the len(rowlabel) do not match known_years. Effectively this test checks that there are as many parameter values as there are known years, in this case through 2018. I am a bit confused by the logic written into the test for certain parameters. See the relevant block of code below:

   # form_parameters are those whose value is available only on IRS form
    form_parameters = ['_AMT_em_pe',
                       '_ETC_pe_Single',
                       '_ETC_pe_Married']
    if param['cpi_inflated']:
        error = False
        known_years = num_known_years
        if pname in long_params:
            known_years = long_known_years
        if pname in form_parameters:
            if len(rowlabel) != (known_years - 1):
                error = True
        else:
            if pname == '_SS_Earnings_c':
                if len(rowlabel) != known_years + 2:
                    error = True
            else:
                if len(rowlabel) != known_years:
                    error = True

If possible, could you please explain why those four parameters (_AMT_em_pe, _ETC_pe_Single, _ETC_pe_Married, and _SS_Earnings_c) have different rules than the other parameters, and what I can do to fix this issue?

@martinholmer
Copy link
Collaborator

martinholmer commented Jan 31, 2019

@Peter-Metz, Sounds like you're making great progress and even learning to ignore my advice when its bad advice.

The test that is puzzling you has to do with the fact that in the past a few parameter values were not (as far as we could tell) available in advance documents and had to be updated only after the IRS forms were published. Hence a few had one less year label. But in the update you're doing we have all the 2018 values and so there is no need to treat a few parameters differently. If you set the form_parameters list to the empty list, can you pass that test?

@Peter-Metz
Copy link
Contributor Author

@martinholmer:

But in the update you're doing we have all the 2018 values and so there is no need to treat a few parameters differently. If you set the form_parameters list to the empty list, can you pass that test?

Thanks for the explanation. Indeed, when I set the form_parameters list to an empty list, the test passes for those three parameters. However, I'm still not sure why the test is looking for _SS_Earnings_c values through 2020. As it stands, we have values through 2019. Is it safe to change the test to reflect this?

Another issue came up as I was going through test failures: _ID_statelocaltax_c and _ID_RealEstate_c , the maximum deduction allowed for state and local income and real estate tax respectively, have not yet been updated to reflect TCJA. As it stands now, values for these two parameters only go through 2017, but as I understand TCJA, the new law limits the deduction of state and local income, sales, and property tax to $10,000 in total (see 1040 Instructions, page 6). Since, in the new law, state/local income and property taxes are combined, but in policy_current_law.json the parameters are separate and in calcfunctions.py the capped amounts are calculated separately, I suspect that we will need to tweak one or both of those files.

Let me know if you think this observation warrants its own issue and/or PR.

@martinholmer
Copy link
Collaborator

@Peter-Metz said:

Indeed, when I set the form_parameters list to an empty list, the test passes for those three parameters. However, I'm still not sure why the test is looking for _SS_Earnings_c values through 2020. As it stands, we have values through 2019.

Try removing the 2019 year and its value from the _SS_Earnings_c record in the policy_current_law.json file. And then look at the failing test to see if you can remove any code that treats the _SS_Earnings_c parameter differently than the other policy parameters.

@martinholmer
Copy link
Collaborator

@Peter-Metz said in #2212:

Another issue came up as I was going through test failures: _ID_statelocaltax_c and _ID_RealEstate_c , the maximum deduction allowed for state and local income and real estate tax, respectively, have not yet been updated to reflect TCJA. As it stands now, values for these two parameters only go through 2017, but as I understand TCJA, the new law limits the deduction of state and local income, sales, and property tax to $10,000 in total (see 1040 Instructions, page 6). Since, in the new law, state/local income and property taxes are combined, but in policy_current_law.json the parameters are separate and in calcfunctions.py the capped amounts are calculated separately, I suspect that we will need to tweak one or both of those files.

Good catch! I agree that this issue should dealt with separately from this pull request.
Can you open a new issue describing this problem? Thanks.

But we have to make some kind of change in order to get the tests to pass for pull request #2212 , right?
Seems like we're in a bind here.

@martinholmer
Copy link
Collaborator

martinholmer commented Feb 1, 2019

@Peter-Metz, After removing the 2019 year and value for the _SS_Earnings_c parameter and hopefully passing that test, can you commit all the local changes on your 2018_params development branch and do a git push origin 2018_params command so that we can see the current status of PR #2212 on GitHub?

@martinholmer
Copy link
Collaborator

@Peter-Metz, Actually things are not a bad as they seem with respect to the limitation of itemized deductions of state/local taxes. The two parameter you mentioned seem not to be used in current (post-TCJA) law. The parameter doing the limiting seems to be this one:

   "_ID_AllTaxes_c": {
        "long_name": "Ceiling on the amount of state and local income, sales and real estate tax deductions allowed (dollars)",
        "description": "The amount of state and local income, sales and real estate tax deductions is limited to this dollar amount.",
        "section_1": "Itemized Deductions",
        "section_2": "State And Local Income And Sales Taxes",
        "irs_ref": "",
        "notes": "",
        "row_var": "FLPDYR",
        "row_label": ["2013",
                      "2014",
                      "2015",
                      "2016",
                      "2017",
                      "2018",
                      "2019",
                      "2020",
                      "2021",
                      "2022",
                      "2023",
                      "2024",
                      "2025",
                      "2026"],
        "start_year": 2013,
        "cpi_inflatable": true,
        "cpi_inflated": true,
        "col_var": "MARS",
        "col_label": ["single", "joint", "separate", "headhousehold", "widow"],
        "boolean_value": false,
        "integer_value": false,
        "value": [[9e99, 9e99, 9e99, 9e99, 9e99],
                  [9e99, 9e99, 9e99, 9e99, 9e99],
                  [9e99, 9e99, 9e99, 9e99, 9e99],
                  [9e99, 9e99, 9e99, 9e99, 9e99],
                  [9e99, 9e99, 9e99, 9e99, 9e99],
                  [10000.0, 10000.0, 5000.0, 10000.0, 10000.0],
                  [10229.0, 10229.0, 5114.5, 10229.0, 10229.0],
                  [10432.56, 10432.56, 5216.28, 10432.56, 10432.56],
                  [10666.25, 10666.25, 5333.12, 10666.25, 10666.25],
                  [10908.37, 10908.37, 5454.19, 10908.37, 10908.37],
                  [11151.63, 11151.63, 5575.81, 11151.63, 11151.63],
                  [11394.73, 11394.73, 5697.37, 11394.73, 11394.73],
                  [11639.72, 11639.72, 5819.86, 11639.72, 11639.72],
                  [9e99, 9e99, 9e99, 9e99, 9e99]],
        "range": {"min": 0, "max": 9e99},
        "out_of_range_minmsg": "",
        "out_of_range_maxmsg": "",
        "out_of_range_action": "stop",
        "compatible_data": {"puf": true, "cps": true}
    },

Can you check the code in the calcfunctions.py file to make sure that this parameter is being used to do the itemized deduction limitation? Thanks.

@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2212   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          12      12           
  Lines        2957    2957           
======================================
  Hits         2957    2957
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 3e71828...5deb032. Read the comment docs.

@Peter-Metz
Copy link
Contributor Author

Peter-Metz commented Feb 1, 2019

@martinholmer:

With the new commits, the PR should pass all tests.

But we have to make some kind of change in order to get the tests to pass for pull request #2212 , right?

For now, I set 2018 values for both _ID_StateLocalTax_c and _ID_RealEstate_c to $10,000 ($5,000 if married filing separately). This is still not technically correct but serves as a placeholder until we can come up with a better solution. Do you think this is a reasonable approach?

To get one of the tests to pass, I had to add a number of 2018 parameter values to policy_current_law.json that were previously not populated (this involved mostly adding 0's and 9e99's). I would appreciate a brief review of the newest additions.

@martinholmer
Copy link
Collaborator

@Peter-Metz, What about this comment? Did you not read it?

@Peter-Metz
Copy link
Contributor Author

@martinholmer, I agree with you assessment in this comment -- that calcfunctions.py uses _ID_AllTaxes_c to calculate maximum itemized deductions. See the block of relevant code from calcfunctions.py below:

c18400 = min(c18400, ID_StateLocalTax_crt * max(c00100, 0.0001))
c18500 = min(c18500, ID_RealEstate_crt * max(c00100, 0.0001))
c18300 = min(c18400 + c18500, ID_AllTaxes_c[MARS - 1])

Still, we need to populate 2018 values for _ID_StateLocalTax_c and _ID_RealEstate_c. Do you think we should continue populating with 9e99's instead of what I have done? My intuition is that the results will be the same, but that may be misguided.

@martinholmer
Copy link
Collaborator

@Peter-Metz said in PR #2212:

Still, we need to populate 2018 values for _ID_StateLocalTax_c and _ID_RealEstate_c.

You mean in order to pass the tests?

Do you think we should continue populating with 9e99's instead of what I have done? My intuition is that the results will be the same, but that may be misguided.

Yes, just put their 2018 values at 9e99 (essentially infinity). I agree with your intuition, but making these changes and rerunning the tests will provide the evidence that either confirms or rejects our intuition.

@martinholmer
Copy link
Collaborator

@Peter-Metz, After doing this don't forget to ./gitsync on your master branch and then merge the updated master branch into your development branch. Your development branch is several commits behind the GitHub master branch as can be seen at https://github.com/PSLmodels/Tax-Calculator/network

@Peter-Metz
Copy link
Contributor Author

@martinholmer, thanks for the reminder. I have updated _ID_StateLocalTax_c and _ID_RealEstate_c per the conversation above. As expected, the results did not change.

@martinholmer
Copy link
Collaborator

@Peter-Metz, Thank your for your contribution in pull request #2212. This was a big project and you have done a very good job, not only in #2212 but also by focusing us on the need for changes in the area of child/dependent credits. Your observations on this later topic in issue #2197 have lead to merged pull requests #2205 and #2211, and to the parameter-renaming discussion in issue #2215 that will lead to another pull request.

But apparently you didn't run the requires_pufcsv tests (as would be done when using the make pytest command) on your local computer after merging recent changes on the master branch into your development branch. If you had you would have seen that the new test_round_trip_tcja_reform test failed. I'll fix that later. Meanwhile I'm merging this pull request into the master branch.

@MattHJensen @codykallen @andersonfrailey @hdoupe

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.

2 participants