Broken TaxBrain links

[martinholmer]

Consider this part of the main TaxBrain page:

The first link (Federal ... for example reforms), the fourth link (Blow-up factors), and the fifth link (Aggregate and distributional targets) are all broken. That is, they each generate an HTML 404 error page.

@MattHJensen @hdoupe @GoFroggyRun @brittainhard

[hdoupe]

The first link indicates that there should be a reform_results.txt file in taxcalc/comparisons. I don't see this. Do we know where that file is located?

The fourth link (blow-up factors) indicates that there should be a Stage_I_factors.csv file in taxdata/Stage II. It looks like the link we are looking for is https://github.com/open-source-economics/taxdata/blob/master/stage1/Stage_I_factors.csv.

The fifth link (Aggregate and distributional targets) indicates that there should be a Stage_II_targets.csv file in taxdata/Stage II. It looks like the link we are looking for is https://github.com/open-source-economics/taxdata/blob/master/stage1/Stage_II_targets.csv.

Can we confirm this?

[hdoupe]

This should be a simple fix if all I have to do is swap out the links in templates/pages/docs.html.

@brittainhard Is that the case here?

[Amy-Xu]

@hdoupe You're right about the factors and targets links. The reform_results.txt was in taxcalc/comparisons until Aug 2. It seems it has become a part of the test suite.

@MattHJensen It seems the dummy datasets item is not relevant anymore. Maybe it can be replaced by CPS dataset, and CPS results. Couldn't remember the last item - maybe it should be deleted.

[hdoupe]

@Amy-Xu great, thanks!

[hdoupe]

@Amy-Xu was reform_results.txt deleted? I don't see it in the test suite.

[Amy-Xu]

@hdoupe I haven't located the file - my guess (from the pr commit) is that one of the tests calculates the changes for reforms and checks the size of changes without generating the file. @martinholmer Martin might have a better sense on this issue.

[martinholmer]

@Amy-Xu said:

I haven't located the file - my guess (from the pr commit) is that one of the tests calculates the changes for reforms and checks the size of changes without generating the file.

Yes, that is correct. See Tax-Calculator pull request 1497 that was merged on August 3rd.

[hdoupe]

@Amy-Xu @martinholmer ok that make sense. So should I just delete the reference to reform_results.txt? Or is there anything that we want to replace it with?

[martinholmer]

@hdoupe asked:

ok that make sense. So should I just delete the reference to reform_results.txt? Or is there anything that we want to replace it with?

Probably "just delete the reference" unless people think it would be useful to link to the taxcalc/tests/test_reforms.py file.

[hdoupe]

@martinholmer Ok, I could do that. It seems like taxcalc/tests/test_reforms.py contains tests for stability. So, alternatively, we could add a statement like:

"Stability tests guarantee that results will not regress over time."

[Amy-Xu]

The test_results.txt was scraped out of JCT tax expenditure report based on the functionality of TC back in 2015. Originally this text was created as an example to demonstrate what TC could do and how close TC results are to JCT and CBO estimates. These results would be beneficial in my opinion because many of these provisions are still relevant and provide users a comparison with JCT/CBO analysis.

@martinholmer @MattHJensen

[martinholmer]

@Amy-Xu said:

The test_results.txt was scraped out of JCT tax expenditure report based on the functionality of TC back in 2015. Originally this text was created as an example to demonstrate what TC could do and how close TC results are to JCT and CBO estimates. These results would be beneficial in my opinion because many of these provisions are still relevant and provide users a comparison with JCT/CBO analysis.

If my memory is correct, the taxcalc/tests/test_reforms.py file contains that information.

@Amy-Xu, is there information that was in test_results.txt but is no longer in test_reforms.py?

[Amy-Xu]

@martinholmer My point is to keep the link. When I replied, I didn't see @hdoupe Hank's proposal. My additional point is that the expenditure results also have demonstration value in themselves besides being a part of stability tests.

[hdoupe]

@Amy-Xu That makes sense to me. I didn't know that the saved results had any value except for ensuring that our results did not regress. So, do we link to the reform_results.py file?

[Amy-Xu]

@hdoupe I think a link to taxcalc/tests/test_reforms.py would do, because as Martin @martinholmer said, it has all the numbers and descriptions.

[hdoupe]

@Amy-Xu Ok, that's fine with me. My only concern is that linking to a python file could be a little intimidating for someone without much Python or coding experience. Do you think it would be worth putting the results in a CSV file?

@martinholmer

[Amy-Xu]

@hdoupe That's a legit concern to me. I think it would be better for TaxBrain users to see the results in a more straightforward form. But the question is where to put that CSV file? If we put it in TC, it's duplicate information with regard to taxcalc/tests/test_reforms.py. I guess that was why Martin @martinholmer removed it in the first place.

[MattHJensen]

Have we done a data update since test_reforms.py was introduced? How is it updated when every reform result changes? I wonder if another format would also be better for that purpose, but I may be missing something.

[martinholmer]

@MattHJensen asked three questions:

[1] Have we done a data update since test_reforms.py was introduced?
[2] How is it updated when every reform result changes?
[3] I wonder if another format would also be better for that purpose?

[1] No puf.csv changes since early August, if my memory is correct.

[2] The test_reforms.py file would be updated just like any other pytest file.

[3] If people are worried about TaxBrain users ability to read the English in the test_reforms.py file, then create another file in the PolicyBrain repo and maintain it so that it is always up-to-date with the test_reforms.py file contents. The approach would have some (readability) benefits but involve maintenance costs for PolicyBrain developers.

@Amy-Xu @hdoupe

[martinholmer]

Just to be clear about the "readability" issues in the test_reforms.py file, here is the test for the second of the 55 tests:

@pytest.mark.requires_pufcsv
def test_r2(puf_subsample):
    """
    Comparison test 2
    """
    reform_json = """
    {
        "start_year": 2015,
        "value": {"_SS_Earnings_c": [177500]},
        "name": "Increase OASDI maximum taxable earnings to $177,500",
        "output_type": "payrolltax",
        "compare_with": {"Budget Options": [40, 46, 49, 51]}
    }
    """
    expected_res = ('Increase OASDI maximum taxable earnings to $177,500'
                    '\n'
                    'Tax-Calculator,48.7,57.3,53.4,55.1')
    actual_res = reform_results(json.loads(reform_json), puf_subsample)
    assert actual_res == expected_res

So, like all the other reform tests, this one has a clear description of the nature and timing of the reform and a clear comparison of Tax-Calculator results and other results (in this case "Budget Options" results).

@MattHJensen @Amy-Xu @hdoupe

[MattHJensen]

My 2c is that readability doesn't seem worth the extra effort of maintaining another file (or code to automatically generate another file) in PolicyBrain, and that we should just link to the test_reforms.py test file for now. test_reforms.py is quite readable already, as @martinholmer points out, so long as you're not scared by code.

That said, it may be worth revisiting this issue after the next data update. I may be shortsighted, but I think updating all 55+ tests manually might prove to be onerous. If so, moving to an approach along the lines of pufcsv_*_actual.txt and pufcsv_*_expect.txt could improve both updatability and readability. But this is all speculative until after the data update.

[hdoupe]

@MattHJensen said:

My 2c is that readability doesn't seem worth the extra effort of maintaining another file (or code to automatically generate another file) in PolicyBrain, and that we should just link to the test_reforms.py test file for now. test_reforms.py is quite readable already, as @martinholmer points out, so long as you're not scared by code.

I agree with this. An alternative to the current approach that may help with readability and may also be more efficient is to have a single json file that stores all of the reforms like:

{
reform0: {
        "start_year": 2015,
        "value": {"_SS_Earnings_c": [177500]},
        "name": "Increase OASDI maximum taxable earnings to $177,500",
        "output_type": "payrolltax",
        "compare_with": {"Budget Options": [40, 46, 49, 51]},
        "expected_result": "'Increase OASDI maximum taxable earnings to $177,500'
                    '\n'
                    'Tax-Calculator,48.7,57.3,53.4,55.1'"
    }, 
refiorm1: {...
    },
...
}

Then, you could have one function that loops over all of the reforms and checks the result. Further, it would be easier to update the expected results or extract them and place them in a CSV or text file.

But as @MattHJensen said we can revisit this after the next data update. For now, I will link to test_reforms.py.

[martinholmer]

@MattHJensen and @hdoupe followed a logical path of thinking about the reform tests that ended with this:

Then, you could have one function that loops over all of the reforms and checks the result. Further, it would be easier to update the expected results or extract them and place them in a CSV or text file.

Yes, this is all true and would be slightly easier to update expected results when a new puf.csv becomes available. But this approach has one very large cost that must be paid every time the tests are run (which for me is many times a day): making this a single test (instead of 55 separate tests) increases the time to run the tests by many minutes, as was described in Tax-Calculator PR1497. The resulting slow-down in development work is a major problem.

[hdoupe]

@martinholmer This is a valid concern. I think this could be a good use case for a parameterized fixture. We could do something like:

@pytest.fixture(scope="module")
def loaded_reform():
     returns list of dictionaries loaded from reforms json file

@pytest.fixture(scope="module", params=[0,...55])
def reform_i(loaded_reform):
    returns loaded_reform[param]

def test_reform(reform_i):
    do test

[martinholmer]

@hdoupe said about the excessive run time of an all-reforms in one test approach:

This is a valid concern.
I think this could be a good use case for a parameterized fixture.
We could do something like:

@pytest.fixture(scope="module")
def loaded_reform():
    returns list of dictionaries loaded from reforms json file

@pytest.fixture(scope="module", params=[0,...55])
def reform_i(loaded_reform):
   returns loaded_reform[param]

def test_reform(reform_i):
   do test

Thanks for the suggestion. Very clever approach. I'll look into it next week.

[hdoupe]

Thanks @martinholmer

[martinholmer]

The TaxBrain links discussed in issue #649 seem to have been removed from TaxBrain 1.0.3.

I assume they will be re-introduced when using Tax-Calculator version >=0.12.0. When re-introducing, be sure to double-check the new file locations in the Tax-Calculator repository.

@MattHJensen @hdoupe @GoFroggyRun

[martinholmer]

Many of the links in TaxBrain (see this comment from almost 14 months ago) are still broken, in that clicking on them produces a 404 error page.

If there is so little interest in making the links to this information work, why not remove all the text that contains the broken links.

@MattHJensen @hdoupe

[hdoupe]

@martinholmer Can you provide the correct links? I'm happy to replace them.

[martinholmer]

@hdoupe said in PolicyBrain issue #649:

Can you provide the correct links? I'm happy to replace them.

I suggest you provide one link to the top-level documentation file that contains links to all the other documentation files, which will be maintained by Tax-Calculator developers:

https://github.com/PSLmodels/Tax-Calculator/blob/master/README.md#tax-calculator

If TaxBrain wants to include other links, that's fine, but don't expect Tax-Calculator developers to maintain those other TaxBrain links.

[hdoupe]

Thanks for the suggestion @martinholmer. How's this (using the link that you provided)?

[martinholmer]

@hdoupe asked:

How's this (using the link that you provided)?

Fine.