Add missing variables to distribution table (with update to TC 0.17.0)

[GoFroggyRun]

This PR closes #771. Before this PR, 'Expanded Income' and 'After-Tax Expanded Income' are not shown in the distribution table, since they were not being rendered into the database.

After this PR, the two are now included in distribution tables, as well as in the "Select Columns" widget:

and

This PR can be viewed as a simple experiment to show that the "Select Columns" widget won't be a problem when new columns are added to the tables, since the widget would render whatever columns the data frame has.

[martinholmer]

PR #848 is incorrect given the merge of Tax-Calculator pull request 1919 because it doesn't add the new UBI column.

[GoFroggyRun]

Looks like having extra columns would fail some of the test suites and yield index error, which I wasn't able to resolve. I'm confused that why some of the tests would fail even if I don't have any problems submitting works via GUI on my local (for both static and dynamic). @hdoupe do you have any ideas how to deal with these errors?

[hdoupe]

Hmmm I think the tests are failing because the mock compute object returns old results. Since we are going to temporarily drop backwards compatibility, I think we should replace the old mock results with the new ones (just use the results from some random run with the current table format). We can retrieve the old results from a previous PolicyBrain future in the future.

[hdoupe]

@martinholmer said:

PR #848 is incorrect given the merge of Tax-Calculator pull request 1919 because it doesn't add the new UBI column.

This can be corrected once PolicyBrain updates to TC 0.17.0, right?

[GoFroggyRun]

@hdoupe said:

Hmmm I think the tests are failing because the mock compute object returns old results.

Ahhh, I see. Then this makes sense: I'm not surprised to see that adding more columns would break the backward compatibilities (which is the case on my local as well).

Since we are going to temporarily drop backwards compatibility, I think we should replace the old mock results with the new ones (just use the results from some random run with the current table format).

Agree. Do you think we should wait until all batches of variables have been added?

[martinholmer]

@hdoupe asked:

This can be corrected once PolicyBrain updates to TC 0.17.0, right?

Yes.

[hdoupe]

Thanks @martinholmer.

[hdoupe]

@GoFroggyRun asked:

Agree. Do you think we should wait until all batches of variables have been added?

I think that we should update the tests as we go. Several PR's will be required to do this update and I think it will be useful to keep track of the different table results. This should be pretty easy to do. Changing this snippet of code to:

        for result in ans:
            year = result['diff_ptax_xdec'].keys()[0][-1]
            curdir = os.path.abspath(os.path.dirname(__file__))
            out = os.path.join(curdir, 'tests/response_year_{}.json'.format(year))
            with open(out, 'w') as w:
                w.write(json.dumps(result))
            for name in results:
                results[name].update(result[name])

will automatically generate updated table results.

Additionally, you will have to xfail a few tests that are set up to parse the older data.

Does that sound sensible?

[GoFroggyRun]

@hdoupe said:

I think that we should update the tests as we go. Several PR's will be required to do this update and I think it will be useful to keep track of the different table results.

The approach sounds sensible to me. However, after changing this snippet of code, I still wasn't able to have all tests passed. In particular, the same problem remains (list index out of range).

Looking at the tests directory after the code change, only response_year_1.json and response_year_2.json got updated, and the rest JSON files remain unmodified (which doesn't make much sense since the code change should have updated all of them), and this confuses me. @hdoupe do you have any ideas why is this happening?

Attached is my test report:

(aei_dropq) Sean-Wangs-MBP:PolicyBrain seanwang$ py.test webapp/apps/
================================== test session starts ===================================
platform darwin -- Python 2.7.14, pytest-3.4.2, py-1.5.2, pluggy-0.6.0
Django settings: webapp.settings (from ini file)
rootdir: /Users/seanwang/Documents/GIT/PolicyBrain, inifile: pytest.ini
plugins: django-3.1.2, celery-4.1.0
collected 186 items                                                                      

webapp/apps/btax/tests/test_views.py .......                                       [  3%]
webapp/apps/dynamic/tests/test_all.py .                                            [  4%]
webapp/apps/dynamic/tests/test_behavioral.py FFFF                                  [  6%]
webapp/apps/dynamic/tests/test_elasticity.py FFx                                   [  8%]
webapp/apps/dynamic/tests/test_models.py ....                                      [ 10%]
webapp/apps/dynamic/tests/test_ogusa.py xxxxxxx                                    [ 13%]
webapp/apps/dynamic/tests/test_views.py ....                                       [ 16%]
webapp/apps/taxbrain/tests/test_all.py .........                                   [ 20%]
webapp/apps/taxbrain/tests/test_compute.py .                                       [ 21%]
webapp/apps/taxbrain/tests/test_helpers.py ....................................... [ 42%]
..............................                                                     [ 58%]
webapp/apps/taxbrain/tests/test_models.py ........                                 [ 62%]
webapp/apps/taxbrain/tests/test_views.py F.FFF..FFFF.x..F.FFxFFFFFFFFF.F..F        [ 81%]
webapp/apps/test_assets/test_param_formatters.py ................................. [ 98%]
..    

[hdoupe]

@GoFroggyRun Strange, it worked for me. I pushed the updated test data as well as some other changes from PR #846 and changes to finish updating to tc 0.17.0. This test data contains tables with all of the data except for the age breakdown.

[GoFroggyRun]

Strange indeed. I copied the same code in the commit c37dcf0 for another try, but still only got response_year_1.json and response_year_2.json updated.

@hdoupe Thanks for the updated test data in changes to finish updating to TC 0.17.0., all valid tests pass using those updated test data.

[hdoupe]

@GoFroggyRun said:

Strange indeed. I copied the same code in the commit c37dcf0 for another try, but still only got response_year_1.json and response_year_2.json updated.

Yes, are you sure that it's calculating all 10 years?

No problem. I think this will be good to go once we update the row labels in the java script like in #846 (taxbrain-tablebuilder.js) However, we want to use the names just like they are in Tax-Calculator which can be found here. @GoFroggyRun would you mind taking care of this last part? Then we can close #846 and just use this PR.

[GoFroggyRun]

@hdoupe said:

I think this will be good to go once we update the row labels in the java script like in #846 (taxbrain-tablebuilder.js) However, we want to use the names just like they are in Tax-Calculator which can be found here. @GoFroggyRun would you mind taking care of this last part? Then we can close #846 and just use this PR.

Sure. So basically I'd incorporate commits/changes in #846, but use updated row names to hardcode the javascript part, would that be sufficient?

[hdoupe]

Yep, you got it. Thanks @GoFroggyRun

[GoFroggyRun]

Here is how the tables look like after commit 1b43d82:

Bin-labled distribution table:

Decile-labled distribution table:

@hdoupe could you review?

[hdoupe]

LGTM. Thanks @GoFroggyRun