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

Fix logic of the run_nth_year_gdp_elast_model function #1677

Merged
merged 6 commits into from
Nov 18, 2017
Merged

Fix logic of the run_nth_year_gdp_elast_model function #1677

merged 6 commits into from
Nov 18, 2017

Conversation

martinholmer
Copy link
Collaborator

This pull request fixes a bug identified by @hdoupe in PolicyBrain 738.

This pull request adds a test to test_tbi.py that fails on the master branch (recreating the error reported by @hdoupe) and passes on this branch, which includes a one-line change to therun_nth_year_gdp_elast_model function.

Thanks, @hdoupe, for the concise bug report.

@MattHJensen @Amy-Xu @andersonfrailey @GoFroggyRun

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1677   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        2856    2880   +24     
======================================
+ Hits         2856    2880   +24
Impacted Files Coverage Δ
taxcalc/policy.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 ff10abb...bdd08c7. Read the comment docs.

@@ -177,7 +177,7 @@ def run_nth_year_gdp_elast_model(year_n, start_year,

# calculate gdp_effect
fyear = check_years_return_first_year(year_n, start_year, use_puf_not_cps)
if (start_year + year_n) > fyear:
if year_n > 0 and (start_year + year_n) > fyear:
# create calc1 and calc2 calculated for year_n - 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@martinholmer what condition are you checking here if (start_year + year_n) > fyear:?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

year_n can be zero even if (start_year + year_n) is greater than fyear,
which is equal to the value returned by this function:

def check_years_return_first_year(year_n, start_year, use_puf_not_cps):
    """
    Ensure year_n and start_year values are valid given input data used.
    Return value of first year, which is maximum of first records data year
    and first policy parameter year.
    """

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see. Thanks.

@hdoupe
Copy link
Collaborator

hdoupe commented Nov 17, 2017

@martinholmer this looks good. Thanks for the quick turn around.

RELEASES.md Outdated
@@ -22,9 +22,6 @@ for a complete commit history.
- Add new policy parameter that changes the stacking order of child/dependent credits
[[#1676](https://github.com/open-source-economics/Tax-Calculator/pull/1676)
by Matt Jensen as suggested by Cody Kallen]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this read:
by Matt Jensen as suggested by Cody Kallen and with need identified by JEC staff.

Copy link
Contributor

Choose a reason for hiding this comment

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

or better yet, 'Joint Economic Committee staff'

@MattHJensen
Copy link
Contributor

Thanks for the latest commit @martinholmer

@martinholmer martinholmer merged commit 4870ea4 into PSLmodels:master Nov 18, 2017
@martinholmer martinholmer deleted the fix-tbi-gdp branch November 19, 2017 20:24
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.

5 participants