Add OBBBA.json and NoOBBBA.json reform files

[maxkossek]

This pull request adds a reform file for the counter-factual of the TCJA expiring and no OBBBA passing, law thus reverting to a pre-TCJA baseline.

[martinholmer]

@maxkossek, Your next step on your local noobbba-reform branch is to move to the top-level (Tax-Calculator) folder of the repository tree and execute the following commands:

(taxcalc-dev) Tax-Calculator>   make cstest
(taxcalc-dev) Tax-Calculator>   make pytest-all

If cstest generates any warnings, fix the code so they go away.
If pytest-all generates any test failures, fix them so they go away.

If you have questions about how to do this, ask for advice here or email me.

ON SECOND THOUGHT Maybe wait to do the above until PR #2942 is merged into the master branch and you have merged that updated master branch into your local noobbba-reform branch.

[jdebacker]

@maxkossek Should this be the current_law_baseline.json?

[maxkossek]

Yes, I've changed Reform_Baseline to policy_current_law.json.

[maxkossek]

If cstest generates any warnings, fix the code so they go away. If pytest-all generates any test failures, fix them so they go away.

@martinholmer, do you all base your out.csv from the reform files based on the output of the res.csv file from running the tests? That is, simply copy the res.csv files to out.csv. This is what I did in commit 8640a0c.

The test_reform_json_and_output() function in test_reforms.py looks like it advances only to 2020, so the results for the OBBBA.json and NoOBBBA.json files are the same (they only changed values for years 2025 onwards). Is this expected, and will be update the tax_year = 2020 in the future to test in the years father in the future?

[martinholmer]

@maxkossek said in PR #2943:

The test_reform_json_and_output() function in test_reforms.py looks like it advances only to 2020, so the results for the OBBBA.json and NoOBBBA.json files are the same (they only changed values for years 2025 onwards). Is this expected, and will be update the tax_year = 2020 in the future to test in the years father in the future?

Excellent point! So the test_reform_json_and_output test is not appropriate for the new JSON reform files. Perhaps treat them the same way ext.json is treated: skip in the test_reform_json_and_output test and add a free-standing test like test_ext_reform for each of the new reforms.
Does that make sense?

[maxkossek]

The test_reform_json_and_output() function in > Excellent point! So the test_reform_json_and_output test is not appropriate for the new JSON reform files. Perhaps treat them the same way ext.json is treated: skip in the test_reform_json_and_output test and add a free-standing test like test_ext_reform for each of the new reforms. Does that make sense?

@martinholmer I've implemented this in c49cd98. What do you think about changing the test_reform_json_and_output to allow for parametrization of the tax_year as regards to the reform files? We would set the default tax_year=2020, but then allow for a different value for the newer files. That is, set tax_year=2026 for ext.json, OBBBA.json, and NoOBBBA.json, and tax_year=2022 for ARPA.json. Here is what this might look like: 311a26c. We could also write more complex code so that the testing function automatically gets the latest year a parameter was set to in the reforms file, and then TaxCalculator runs advance_to_year until this last year. For example, for the OBBBA.json, tax_year=2030 because ID_AllTaxes_c has values set up to 2030. This change would remove the need to write separate test_ext_reform-like functions.

[martinholmer]

@maxkossek said in PR #2943:

What do you think about changing the test_reform_json_and_output to allow for parametrization of the tax_year as regards to the reform files? We would set the default tax_year=2020, but then allow for a different value for the newer files.

I think this is a good idea. Consider specifying a dictionary that has reform-file-name (rname) keys and tax_year values, but include only the files that you want to have tax_year greater than 2020. Then access this dictionary using the NAME_OF_YOUR_DICT.get(rname, 2020) Python function.

You may still want to specify test_noobbba_reform using the always-available CPS data because it is a much stronger test than test_reform_json_and_output.

[maxkossek]

I think this is a good idea. Consider specifying a dictionary that has reform-file-name (rname) keys and tax_year values, but include only the files that you want to have tax_year greater than 2020. Then access this dictionary using the NAME_OF_YOUR_DICT.get(rname, 2020) Python function.

This is added in commit: 8336b8a. My local tests pass.

Marking this PR as ready for @martinholmer @jdebacker to review.

[martinholmer]

@maxkossek, Remember that the Tax-Calculator Python coding style is to use single quotes.

So, the code fragment:

pytest.mark.reforms
@pytest.mark.parametrize(
    "reform_file,tax_year",             <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    [(os.path.basename(f), REFORM_YEARS.get(os.path.basename(f), 2020))
     for f in REFORM_FILES],
)
def test_reform_json_and_output(reform_file, tax_year, tests_path):

should look like this:

@pytest.mark.reforms
@pytest.mark.parametrize(
    'reform_file,tax_year',             <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    [(os.path.basename(f), REFORM_YEARS.get(os.path.basename(f), 2020))
     for f in REFORM_FILES],
)
def test_reform_json_and_output(reform_file, tax_year, tests_path):

[martinholmer]

@maxkossek, The three CPS tests in test_reforms.py (test_ext_reform, test_obbba_reform, test_noobbba_reform) seem to have very similar logic. Is it possible to use the pytest.parameterize capability to consolidate these three test functions into one?

[maxkossek]

@maxkossek, Remember that the Tax-Calculator Python coding style is to use single quotes.

@maxkossek, The three CPS tests in test_reforms.py (test_ext_reform, test_obbba_reform, test_noobbba_reform) seem to have very similar logic. Is it possible to use the pytest.parameterize capability to consolidate these three test functions into one?

Thank you @martinholmer, I've made these changes in: 20e46ff. To enforce the coding style, do you want me to add pylint-quotes to enforce single quotes in the cstest?

[martinholmer]

@maxkossek asked in PR #2943:

To enforce the [single-vs-double-quote Python] coding style, do you want me to add pylint-quotes to enforce single quotes in the cstest?

Thanks for pointing out the existence of this pylint plugin. Let's think about it. You could open a new issue to discuss the benefits and costs of enforcing this coding style. The cost of enforcing this would be non-trivial as shown by these results:

(taxcalc-dev) Tax-Calculator> grep -r --include="*py" \" . | grep -v \"\"\" | wc -l 
    1215

There are a few lines that use double quotes in a situation in which both type of quotes are necessary.
Also, I notice that the bulk of the double quote usage is in the tests folder.
You can leave out the | wc -l part of the pipeline to see which files have the double quotes.

[martinholmer]

@maxkossek, My review PR #2943 is complete. This is very nice work, especially for someone who is relatively new to the Tax-Calculator project.

@jdebacker, We are awaiting your review of PR #2943.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (2261e0d) to head (20e46ff).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2943   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines         2657      2657           
=========================================
  Hits          2657      2657           

Flag	Coverage Δ
unittests	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinholmer]

I approve these changes

[jdebacker]

@maxkossek Excellent work in this PR! Merging.