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

Universal Basic Income Addition - Updated #1919

Merged
merged 6 commits into from
Mar 14, 2018

Conversation

Kpinkelman
Copy link
Contributor

Had an issue with the branch opened for the first PR, so opening a new PR with both the distribution and difference tables edited.

@martinholmer
Copy link
Collaborator

martinholmer commented Mar 12, 2018

@Kpinkelman, pull request #1919 looks fine (assuming the tests pass) except for one thing.
In the difference table you quite sensibly placed the new ubi statistic before* the main summary statistic (the percentage change in aftertax income). But in the distribution table you put the new ubi statistic after the main summary statistics (expanded_income and aftertax_income).

Can you move the ubi statistic in the distribution table so it is just before the expanded_income statistic?
Thanks.

P.S. Looks like you have a PEP8 error to fix anyway.

Reordered Distribution Table
@martinholmer
Copy link
Collaborator

@Kpinkelman, please read about the testing procedures you need to be following before submitting a pull request in this document.

@Kpinkelman
Copy link
Contributor Author

Kpinkelman commented Mar 12, 2018

@martinholmer That's odd - it seemed to be passing tests when I ran it locally:

(taxcalc-dev) OSPC-Intern-MacBook-Air:taxcalc intern$ py.test -m "not requires_pufcsv and not pre_release" -n4
============================== test session starts ===============================
platform darwin -- Python 2.7.14, pytest-3.4.1, py-1.5.2, pluggy-0.6.0
rootdir: /Users/intern/Desktop/tax-calculator, inifile: setup.cfg
plugins: xdist-1.22.1, forked-0.2, pep8-1.0.6
gw0 [321] / gw1 [321] / gw2 [321] / gw3 [321]
scheduling tests via LoadScheduling
.......................................................................... [ 23%]
.......................................................................... [ 46%]
.......................................................................... [ 69%]
.......................................................................... [ 92%]
.........................                                                  [100%]
========================== 321 passed in 178.98 seconds ==========================

and

(taxcalc-dev) OSPC-Intern-MacBook-Air:validation intern$ ./tests.sh
STARTING WITH VALIDATION TESTS : Fri Mar  9 14:51:51 EST 2018
. taxsim/d15
============ ALL EXECUTED TAXSIM VALIDATION TESTS PASS

taxcalc/utils.py Outdated
@@ -86,7 +88,7 @@
# labels list to map a label to the correct column in a difference table.

DIFF_VARIABLES = ['expanded_income', 'c00100', 'aftertax_income',
'iitax', 'payrolltax', 'combined', 's006']
'iitax', 'payrolltax', 'combined', 's006','ubi']
Copy link
Collaborator

@andersonfrailey andersonfrailey Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the reasons the tests are failing is this line. You need to add a space between the comma and 'ubi' to comply with PEP8 standards.

@martinholmer
Copy link
Collaborator

martinholmer commented Mar 12, 2018

@Kpinkelman said:

That's odd - it seemed to be passing tests when I ran it locally.

Yes, that is odd because I get two failures on my computer when testing PR#1919 and that's what happens on GitHub. Here is what I get:

$ cd taxcalc
$ py.test -n4 -m "not requires_pufcsv and not pre_release" 
============================= test session starts ==============================
platform darwin -- Python 2.7.14, pytest-3.2.1, py-1.4.34, pluggy-0.4.0
rootdir: /Users/mrh/work/OSPC/tax-calculator, inifile: setup.cfg
plugins: xdist-1.17.1
gw0 [321] / gw1 [321] / gw2 [321] / gw3 [321]
scheduling tests via LoadScheduling
............................................................................................................................................F.............................F......................................................................................................................................................
=================================== FAILURES ===================================
_________________________ test_validity_of_name_lists __________________________
[gw3] darwin -- Python 2.7.14 /Users/mrh/anaconda/bin/python
def test_validity_of_name_lists():
>       assert len(DIST_TABLE_COLUMNS) == len(DIST_TABLE_LABELS)
E       AssertionError: assert 22 == 21
E        +  where 22 = len(['s006', 'c00100', 'num_returns_StandardDed', 'standard', 'num_returns_ItemDed', 'c04470', ...])
E        +  and   21 = len(['Returns', 'AGI', 'Standard Deduction Filers', 'Standard Deduction', 'Itemizers', 'Itemized Deduction', ...])

tests/test_utils.py:62: AssertionError
__________________________ test_table_columns_labels ___________________________
[gw3] darwin -- Python 2.7.14 /Users/mrh/anaconda/bin/python
def test_table_columns_labels():
        # check that length of two lists are the same
>       assert len(DIST_TABLE_COLUMNS) == len(DIST_TABLE_LABELS)
E       AssertionError: assert 22 == 21
E        +  where 22 = len(['s006', 'c00100', 'num_returns_StandardDed', 'standard', 'num_returns_ItemDed', 'c04470', ...])
E        +  and   21 = len(['Returns', 'AGI', 'Standard Deduction Filers', 'Standard Deduction', 'Itemizers', 'Itemized Deduction', ...])

tests/test_utils.py:968: AssertionError
==================== 2 failed, 319 passed in 136.04 seconds ====================

These are substantive errors that indicate somethings are not quite right in your pull request.
Look at the test code around the two line locations to see why the tests are failing.

@andersonfrailey
Copy link
Collaborator

When running the test locally I get the same two errors as @martinholmer. The errors are popping up because DIST_TABLE_COLUMNS and DIST_TABLE_LABELS are not the same length. I did a little command line work and found this:

In [10]: list(enumerate(DIST_TABLE_COLUMNS))
Out[10]: 
[(0, 's006'),
 (1, 'c00100'),
 (2, 'num_returns_StandardDed'),
 (3, 'standard'),
 (4, 'num_returns_ItemDed'),
 (5, 'c04470'),
 (6, 'c04600'),
 (7, 'c04800'),
 (8, 'taxbc'),
 (9, 'c62100'),
 (10, 'num_returns_AMT'),
 (11, 'c09600'),
 (12, 'c05800'),
 (13, 'c07100'),
 (14, 'othertaxes'),
 (15, 'refund'),
 (16, 'iitax'),
 (17, 'payrolltax'),
 (18, 'combined'),
 (19, 'ubi'),
 (20, 'expanded_income'),
 (21, 'aftertax_income')]

In [11]: list(enumerate(DIST_TABLE_LABELS))
Out[11]: 
[(0, 'Returns'),
 (1, 'AGI'),
 (2, 'Standard Deduction Filers'),
 (3, 'Standard Deduction'),
 (4, 'Itemizers'),
 (5, 'Itemized Deduction'),
 (6, 'Personal Exemption'),
 (7, 'Taxable Income'),
 (8, 'Regular Tax'),
 (9, 'AMTI'),
 (10, 'AMT Filers'),
 (11, 'AMT'),
 (12, 'Tax before Credits'),
 (13, 'Non-refundable Credits'),
 (14, 'Other Taxes'),
 (15, 'Refundable Credits'),
 (16, 'Individual Income Tax Liabilities'),
 (17, 'Payroll Tax Liablities'),
 (18, 'Combined Payroll and Individual Income Tax Liabilities'),
 (19, 'Universal Basic IncomeExpanded Income'),
 (20, 'After-Tax Expanded Income')]

The label for expanded_income is missing from DIST_TABLE_LABELS because there is no comma between Universal Basic Income and Expanded Income in the list. @Kpinkelman, add the missing comma and you should be good.

taxcalc/utils.py Outdated
@@ -78,6 +79,7 @@
'Individual Income Tax Liabilities',
'Payroll Tax Liablities',
'Combined Payroll and Individual Income Tax Liabilities',
'Universal Basic Income'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kpinkelman, add the comma here.

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1919    +/-   ##
=======================================
  Coverage     100%    100%            
=======================================
  Files          37      38     +1     
  Lines        3319    3588   +269     
=======================================
+ Hits         3319    3588   +269
Impacted Files Coverage Δ
taxcalc/utils.py 100% <100%> (ø) ⬆️
taxcalc/parameters.py 100% <0%> (ø) ⬆️
taxcalc/__init__.py 100% <0%> (ø) ⬆️
taxcalc/simpletaxio.py 100% <0%> (ø)

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 14b5637...37c8ea0. Read the comment docs.

@martinholmer martinholmer merged commit d44a7d5 into PSLmodels:master Mar 14, 2018
@martinholmer
Copy link
Collaborator

@Kpinkelman, Thanks for the contribution in pull request #1919.

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.

4 participants