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

Add test that shows bug in GainsTax function and fix bug #981

Merged
merged 5 commits into from
Oct 12, 2016
Merged

Add test that shows bug in GainsTax function and fix bug #981

merged 5 commits into from
Oct 12, 2016

Conversation

martinholmer
Copy link
Collaborator

This pull request adds a test to illustrate a bug in the GainsTax function and then fixes that bug. This bug was reported by @codykallen in issue #977. The fix for this bug in this pull request is logically equivalent to the code revision suggested by @codykallen in pull request #979, but eliminates the duplication of code inherent in #979. Thank you, @codykallen, for your helpful bug report in #977 and your correct diagnosis of the bug's cause in #979.

As an aside for other Tax-Calculator contributors, the bug report in #977 is an example to be emulated: it describes the problem and provides a simple test to demonstrate the problem. #979 goes the extra mile by actually suggesting how to fix the bug reported in #977.

What I have done in this pull request is two things:
(1) created a new unit test based on the bug report in #977, and
(2) fixed the bug (that is, revised the code so that the new test does not fail).

@MattHJensen @feenberg @Amy-Xu @GoFroggyRun @andersonfrailey @zrisher

@codecov-io
Copy link

codecov-io commented Oct 12, 2016

Current coverage is 98.27% (diff: 100%)

Merging #981 into master will not change coverage

@@             master       #981   diff @@
==========================================
  Files            35         35          
  Lines          2085       2085          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           2049       2049          
  Misses           36         36          
  Partials          0          0          

Powered by Codecov. Last update 9b67e16...e4618b6

@martinholmer
Copy link
Collaborator Author

martinholmer commented Oct 12, 2016

Before the #981 bug-fix code change in the functions.py file, the results of the new test_Calculator_mtr_when_PT_rates_differ test were as follows:

>       assert np.allclose(mtr1, mtr2, rtol=0.0, atol=1e-06)
E       assert <function allclose at 0x102f93410>(array([ 0.188]), array([ 0.438]), rtol=0.0, atol=1e-06)
E        +  where <function allclose at 0x102f93410> = np.allclose

So, before the bug fix, the marginal tax rate on LTCG (p23250) was different before and after the reform that made pass-through tax rates different from tax rates on non-pass-through income: instead of being the same, the mtr on LTCG was 0.188 (the 15% CG rate plus the 3.8% NIIT rate) when II and PT tax rates were identical and 0.438 (the 40% II rate plus the 3.8% NIIT rate) after the reform that made all II rates equal to 40% and all PT rates equal to 30%.

After the bug fix in #981, the mtr on LTCG is 0.188 both before and after the reform that makes II and PT rates different.

@MattHJensen @feenberg @Amy-Xu @GoFroggyRun @andersonfrailey @zrisher

@MattHJensen
Copy link
Contributor

@martinholmer, this looks good to me. @codykallen, thanks a lot for the bug report and suggested fix!

@martinholmer, do you think it is ok to merge this now? I think it deserves a TC version bump and redeploy to TaxBrain asap given that I know there are currently users on TB who would be affected.

cc @talumbau

@martinholmer
Copy link
Collaborator Author

@MattHJensen said:

@martinholmer, this [#981] looks good to me. @codykallen, thanks a lot for the bug report and suggested fix!

@martinholmer, do you think it is ok to merge this now? I think it deserves a TC version bump and redeploy to TaxBrain asap given that I know there are currently users on TB who would be affected.

Yes, I'm comfortable with the changes in pull request #981, but was hoping to get a thumbs up from @codykallen before doing the merge. But I haven't heard anything from his this morning.

@martinholmer
Copy link
Collaborator Author

Merged pull request #981 resolves issue #977 and incorporates the bug-fix strategy (but not tactics) of pull request #979.

@martinholmer martinholmer mentioned this pull request Oct 12, 2016
@martinholmer martinholmer deleted the add-mtr-test branch October 12, 2016 19:58
@martinholmer martinholmer mentioned this pull request Oct 22, 2016
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