Cap state/local deductions as fraction of AGI

[derrickchoe]

I'm adding a reform I made a few months back for a project. I created the parameter StateLocalTax_crt, which caps state/local tax deductions as a decimal fraction of agi.

[codecov-io]

Codecov Report

Merging #1711 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1711   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        3000    3000           
======================================
  Hits         3000    3000

---

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 fb321fc...d3480e3. Read the comment docs.

[MattHJensen]

Could you take a look at _ID_Charity_crt_all (https://github.com/open-source-economics/Tax-Calculator/blob/master/taxcalc/current_law_policy.json#L1262) and model your long_name and description off of that?

[derrickchoe]

I'll be sure to model the ID_StateLocalTax_ct description after that parameter. Thanks!

[MattHJensen]

It looks to me like this line applies the cap to the combination of state and local income and sales taxes and state, local, and foreign real estate taxes. If that is your intent, the description of the parameters should be changed.

[MattHJensen]

I don't think you should end up needing a conditional here given that you set ID_StateLocalTax_crt at 1 under current law.

[derrickchoe]

Thanks for the comments, Matt! I included the conditional to take care of rare cases where one's state and local tax deductions exceed their AGI (I think this happens when someone can write off a large business loss). I had considered setting the parameter at 9e99 under current law, but this still wouldn't take care of cases of itemizers with negative AGI. I'm more than open to suggestions which take care of the if condition.

[MattHJensen]

@derrickchoe, thanks for opening a PR to add this new feature! Just commented a few lines with suggestions.

I also recommend adding a test that confirms setting _ID_StateLocalTax_crt to 0 generates the same results as setting _ID_StateLocalTax_hc to 1. (Or explain why you think those should be different). See https://github.com/open-source-economics/Tax-Calculator/blob/master/taxcalc/tests/test_calculate.py#L255 for an example.

[derrickchoe]

@martinholmer

Thanks for the suggestions, @MattHJensen. I've split the reform parameter into separate state/local and real estate tax deduction ceilings (as fractions of AGI). I've also set the baseline value for these parameters at 9e99, to take care of cases where one's deductions exceed their AGI. I'm having a bit of trouble understanding the issues with my current code. It seems like they stem from utils.py and calculate.py, neither of which I edited.

Is this happening because my taxcalc branch is outdated? If so, I'd appreciate any insight you might have regarding solutions. Thanks!

[martinholmer]

@derrickchoe said:

I'm having a bit of trouble understanding the issues with my current code. It seems like they stem from utils.py and calculate.py, neither of which I edited.

The coverage test is failing because the following two lines are untested.

non_sum_cols = [c for c in diffs.columns if 'perc_' in c]
for col in non_sum_cols:                           <=========== UNTESTED
    diffs.loc['sums', col] = np.nan                <=========== UNTESTED
diffs.loc['sums', 'share_of_change'] = 1.0  # to avoid rounding error

But that's not your fault at all. I think you can ignore that coverage test failure because these two untested lines will be removed soon as part of the resolution in Tax-Calculator of PolicyBrain issue 73.

@derrickchoe also asked:

Is this happening because my taxcalc branch is outdated?

No. You've done a good job. Your confusion is my fault for leaving two untested statements on the master branch. Sorry about that.

[martinholmer]

@derrickchoe asked:

Is this happening because my taxcalc branch is outdated?

Again, I think the answer is no.

But it is true that your branch does not include many recent changes on the master branch. So, for future reference, when you have a long-running development branch on your local computer, it is good practice to incorporate recent master changes into that branch. Do that by following the directions in Workflow Step 6 in the Contributor Guide.

Thanks for your contribution to Tax-Calculator.

[derrickchoe]

@martinholmer, thanks for clarifying. I'll be sure to keep my branches updated with recent changes to the master branch in the future. I appreciate your help!

[martinholmer]

@derrickchoe said:

thanks for clarifying. I'll be sure to keep my branches updated with recent changes to the master branch in the future. I appreciate your help!

Sure, no problem. There are a lot of moving parts involved in making even a modest contribution to Tax-Calculator. You've done very well.

[martinholmer]

@MattHJensen, Is pull request #1711 ready to be merged? If so, go ahead and do that.

[MattHJensen]

@derrickchoe, this is excellent. Thanks.

I do still recommend taking the additional step of adding a couple of tests to ensure that setting _ID_StateLocalTax_crt to 0 generates the same results as setting _ID_StateLocalTax_hc to 1 and same for real estate taxes.

See https://github.com/open-source-economics/Tax-Calculator/blob/master/taxcalc/tests/test_calculate.py#L255 for an example.

Alternatively, you could add these (4) reforms to reforms.json and visually ensure that the results are the same. In general, when we have two parameters that allow us to get at the same reform in different ways, it is a great opportunity to check that everything is working as expected.

cc @martinholmer

[derrickchoe]

@MattHJensen, I'm happy to add those tests (it will be valuable python practice as well). I'll make the changes to test_calculate.py and push them once I'm finished. Thanks!

@martinholmer

[MattHJensen]

I'm happy to add those tests (it will be valuable python practice as well). I'll make the changes to test_calculate.py and push them once I'm finished.

Thanks!

[derrickchoe]

@MattHJensen @martinholmer

I've just gotten to writing in some tests to make sure that capping state/local and real estate tax deductions at 0% of AGI is equivalent to a full haircut of those respective parameters. I wrote two functions in test_calculate.py, modeled after the code you linked.

https://github.com/open-source-economics/Tax-Calculator/blob/c36faaf8b26c40af0762218f17267a049da7e4eb/taxcalc/tests/test_calculate.py#L270

I'm not too familiar with tests-- are there any other files in taxcalc that I should edit so that the tests run properly? Thanks to both of you for your help, and sorry in advance if this info was covered somewhere in the tax calc documentation.

[martinholmer]

@derrickchoe said:

I'm having trouble getting the full tests to work when swapping 0.0001 for 0 in the following code:
...
The issues are coming with the test_pufcsv.py file.

Are you running Python 3.6 (rather than 2.7)?

[martinholmer]

@derrickchoe, I just downloaded PR#1711 and changed the two max(c00100, 0) expressions to max(c00100, 0.0001) per the suggestion made by @codykallen. All the tests (including the requires_pufcsv tests) run without any failures.

Can you make those coding changes (like @codykallen suggested) and commit them and push them, so that I can merge #1711? We'll figure out later what sort of problems you're having with the tests. But both @GoFroggyRun and I have confirmed that the @codykallen revisions eliminate all the test failures that I reported yesterday.

One additional question: when you are not on GitHub would you say your name is Derrick Choe?
If not, what you you say your name is?

[derrickchoe]

@martinholmer

Thanks for asking-- yes my name is Derrick Choe.

In response to:

Are you running Python 3.6 (rather than 2.7)?

I believe I am running Python 3.6. I did install a Python 2.7 environment via Anaconda, though. I'm still familiarizing myself with this, and I'll be sure to confirm exactly with what version of Python I'm running tax-calculator.

In response to:

Can you show us exactly what the differences are between the actual and expected output?

I'll pull up the test error message shortly. I also had a couple of issues with index.html (which I know I can fix by running make_index.py), as well as an error message related to a missing mock module, but I am told that these are not problems with the code itself.

Lastly:

Can you make those coding changes (like @codykallen suggested) and commit them and push them, so that I can merge #1711?

I'll be sure to do that now. Thanks again for all of your guidance with this pull request!

[martinholmer]

Thanks, @derrickchoe, for the enhancements in pull request #1711.

[derrickchoe]

@martinholmer
I just reran the tests using my Python 2.7 environment console. The errors associated with test_pufcsv.py did not show up this time. Sorry about that-- I'll be sure to use Python 2.7 for future tests.

Also, thanks for merging the changes!

[martinholmer]

@derrickchoe said:

I just reran the tests using my Python 2.7 environment console. The errors associated with test_pufcsv.py did not show up this time. Sorry about that-- I'll be sure to use Python 2.7 for future tests.

I think your experience using Python 3.6 is the same as @MattHJensen's experience in #1715. There seems to be slight differences in the floating-point libraries in 2.7 vs 3.6, which produce slightly different numerical results. Can you post the test_pufcsv.py differences you get when running Python 3.6? Are they the same as @MattHJensen reports in #1715?

So, you didn't do anything wrong and there's nothing to be sorry for. Thanks again for this contribution.

[derrickchoe]

@martinholmer I get the following results when I run the test using Python 3.6 (please let me know if the screenshots are too messy-- I'm not exactly sure how to best show failed tests):

Is this the output that you are referring to? Looking at #1715, I am having a bit of trouble identifying exactly which differences are arising in the tests.

[martinholmer]

@derrickchoe, Thanks for the info on test failures under Python 3.6.
Looks like you're working on Windows, right?

When you ran these tests your master branch was up-to-date, right?
Did it include the changes in pull request #1773? If not, please update your master branch so that it includes the 1773 changes.

What's the difference between the pufcsv_agg_expect.txt file and the pufcsv_agg_actual.txt file?
Get the differences in the two files using a diff utility. Working on Windows means you don't have many useful tools available at the command prompt, so you might need to find a Windows diff utility and install it on your computer.

[derrickchoe]

@martinholmer

While I had forgotten to update my master branch with changes in pull request #1773, I am getting the same test failures with my now-updated branch. The files test_pufcsv.py and test_puf_var_stats.py are both showing the same errors. Additionally, I'm getting an issue with test_reforms.py-- I think the issue has to do with the rename function (output below):

In response to:

What's the difference between the pufcsv_agg_expect.txt file and the pufcsv_agg_actual.txt file?
Get the differences in the two files using a diff utility.

I'm not too familiar with diff utilities; I'm looking into installing and using one now. I'll send an update once I have a better understanding of the differences between the two files. Thanks!

[martinholmer]

Thanks for the report on the test failure in test_reforms.py.
Resolve that part of the problem by reading pull request #1781.

[derrickchoe]

@martinholmer @MattHJensen

Sorry in advance for the large screenshot. I'm getting differences between actual and expected output for the following numbers:

2018 AMT liability
2019 itemized deductions and taxable income
2020 taxable income

All of these differences are in the magnitude of 0.1 billion. Is it possible that this is a rounding error?

[martinholmer]

@derrickchoe said:

I'm getting differences between actual and expected output for the following numbers:

2018 AMT liability
2019 itemized deductions and taxable income
2020 taxable income

All of these differences are in the magnitude of 0.1 billion. Is it possible that this is a rounding error?

Yes, I think it is just rounding error caused by differences in the floating-point libraries between Python 2.7 and Python 3.6. We have experienced this before with other tests that use the cps.csv input, which are run under both 2.7 and 3.6 when you create a GitHub pull request.

These test failures are happening even on the master branch, right?

I'll try to generalize test_pufcsv.py so it doesn't fail when actual and expected differ by only $0.1 billion.
When I do that, I'll ask you to run the tests on your computer to see if they all pass. (Remember GitHub doesn't run the requires_pufcsv tests because of data privacy issues.)

Thanks for all the help!

[derrickchoe]

@martinholmer
In response to:

These test failures are happening even on the master branch, right?

That's right; I ran these tests on my up-to-date master branch. Thanks for addressing this; I'll be sure to stay in the loop. I'm happy to run the tests whenever you think is appropriate. In the meantime, I'm glad that the recent additions to tax-calculator are running smoothly! I'm looking forward to making future contributions.

[martinholmer]

@derrickchoe, Can you update your master branch to include several recent merges on GitHub, and then run all the tests including the requires_pufcsv tests. Then let me know if all the 390+ tests pass on your computer. If one or more failed, let me know what the differences are.

Thanks so much in advance for helping me out on this. I don't have Python 3.6, so I'm thankful that you do and that you are willing to run the tests.

[derrickchoe]

@martinholmer

I've updated my master branch, and test_pufcsv.py is working now. However, I'm still getting an error message with test_puf_var_stats.py. I've attached the differences below (they occur in lines 5-6 of puf_var_wght_means_by_year.csv).

I'm happy to help out-- given enough time working with tax-calculator, I hope to be able to help fix minor issues (such as these ones) in the future as well.

[derrickchoe]

@martinholmer
In response to:

If one or more failed, let me know what the differences are.

I should mention that I'm working on my personal computer now, and I noticed that a new test failed (I suspect it just has to do with my installation of taxcalc).

In test_4package.py, I'm getting a "file not found" error for lib\subprocess.py (I don't know if this means that subprocess.py is missing). Is this a common issue?

[martinholmer]

@derrickchoe said:

In test_4package.py, I'm getting a "file not found" error for lib\subprocess.py (I don't know if this means that subprocess.py is missing). Is this a common issue?

No its not common, but more on that below.

My understanding is that subprocess is part of the standard, built-in Python library.
So, if you fail the tests in test_4package.py, then you must also be failing the tests in test_notebooks.py. Is that the case? I'm guessing that because the subprocess library is imported in both those files as seen below:

- grep; default-directory: "~/work/OSPC/tax-calculator/taxcalc/tests/" -*-
grep -nH -e subprocess *
test_4package.py:10:import subprocess
test_4package.py:22:    out = subprocess.check_output(['conda', 'list', 'taxcalc']).decode('ascii')
test_notebooks.py:3:import subprocess
test_notebooks.py:16:    rcode = subprocess.check_call(['python', sfilename], cwd=test_path)

Nobody has ever reported this as a problem, but maybe that's because all the other developers are working on Macs and you're working on Windows. That's just a guess.

Are you executing py.test -n4 from the Windows command prompt or from inside some IDE?

[codykallen]

@martinholmer said:

Nobody has ever reported this as a problem, but maybe that's because all the other developers are working on Macs and you're working on Windows.

I don't think that's the issue. I'm working on Windows, and I've never had this problem.

[martinholmer]

@derrickchoe said:

I've updated my master branch, and test_pufcsv.py is working now. However, I'm still getting an error message with test_puf_var_stats.py. I've attached the differences below (they occur in lines 5-6 of puf_var_wght_means_by_year.csv).

Thanks for the helpful feedback. I'll fix these problems in my next pull request. And then I'm hoping you can run the tests again to see that all the test failures caused by numerical results differences have been eliminated.

[martinholmer]

@codykallen said:

I don't think that's the issue. I'm working on Windows, and I've never had this problem.

OK, I didn't know you were working on Windows, @codykallen. Are you using Python 2.7 or Python 3.6?

So @derrickchoe's problem seems related to the Python 3.6 setup on his personal computer.

[codykallen]

@martinholmer, I use Python 2.7 on my work computer and Python 3.6 on my personal computer.

[martinholmer]

@derrickchoe, Hopefully with the merge of PR #1789 there will be no small numerical differences when you run the full set of tests under Python 3.6. Thanks again for helping me out on this.

[derrickchoe]

@martinholmer in response to:

Are you executing py.test -n4 from the Windows command prompt or from inside some IDE?

I'm executing py.test -n4 from the command line. Specifically, I'm using the taxcalc-dev environment in Anaconda Prompt. For some reason, the -n4 specification only works when working from this environment.

So @derrickchoe's problem seems related to the Python 3.6 setup on his personal computer.

Thanks @codykallen for comparing with your installation-- I'll continue looking into this issue. I'm only failing the test_4package.py test.

[derrickchoe]

@martinholmer

In response to:

@derrickchoe, Hopefully with the merge of PR #1789 there will be no small numerical differences when you run the full set of tests under Python 3.6. Thanks again for helping me out on this.

I'm no longer running into any issues besides the one involving subprocess.py. Thanks for resolving all of the issues that I was running into using python 3.6!