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

Unexpected TaxBrain warning when using valid reform #638

Closed
martinholmer opened this issue Aug 25, 2017 · 14 comments
Closed

Unexpected TaxBrain warning when using valid reform #638

martinholmer opened this issue Aug 25, 2017 · 14 comments
Assignees
Labels

Comments

@martinholmer
Copy link
Contributor

I don't think it is too much of an exaggeration to say that at the moment TaxBrain is unusable.

When I enter this simple reform

screen shot 2017-08-25 at 10 41 27 am

I get this message from TaxBrain

screen shot 2017-08-25 at 10 40 56 am

@brittainhard
Copy link
Contributor

@martinholmer I just ran this reform on ospc.org twice and it worked just fine. Did you have different inputs than just those four?

@martinholmer
Copy link
Contributor Author

martinholmer commented Aug 25, 2017

@brittainhard said:

I just ran this reform on ospc.org twice and it worked just fine. Did you have different inputs than just those four?

I tried it again and got the same 500 Server Error message.

Can you share the TaxBrain run URL that shows the static results from your run?

@brittainhard
Copy link
Contributor

@brittainhard
Copy link
Contributor

Looks like I put year 2017 instead of 2013. I was able to re-create your error.

@martinholmer
Copy link
Contributor Author

martinholmer commented Aug 25, 2017

@brittainhard said he got 2017 results as shown at https://www.ospc.org/taxbrain/16108/

That's good, but that is not the reform I got the error for.
You completely ignored the Start Year of 2013.

Can you raise the standard deduction amounts to $50,000 in 2013 and avoid a TaxBrain crash?
If so, please post the URL of the static results page.

@MattHJensen

@martinholmer
Copy link
Contributor Author

@brittainhard, Looks like our comments to #638 crossed in the delay while GitHub is emailing comments.

So, I guess we both get a 500 Server Error when trying to raise the standard deduction amounts to $50,000 in 2013. Right?

@brittainhard
Copy link
Contributor

@martinholmer thats correct. im looking into this now.

@hdoupe
Copy link
Collaborator

hdoupe commented Sep 28, 2017

Closed via #641. See successful run here

@martinholmer
Copy link
Contributor Author

martinholmer commented Oct 3, 2017

@hdoupe said in #641:

Closed via #641. See successful run here

Actually, this was not a test of the bug originally reported in #641 because the "successful run" used start year 2017 instead of start year 2013.

When I use start year 2013 and raise the standard deduction amount to $50,000 in that year, I get a warning at the top of the GUI input page, but I don't see any red warning messages below any of the input boxes. I get these results when using the 1.0.3 release candidate. Here is what I see at the top of the page:

screen shot 2017-10-03 at 4 58 56 pm

And further down on the same page, I see this:

screen shot 2017-10-03 at 4 59 23 pm

Note that there are no warning messages below the standard deduction input boxes.

I'm not sure what is going on here. If TaxBrain sees a warning message returned from Tax-Calculator, then it should display that warning message below the appropriate parameter input box. That doesn't seem to be happening in this case, so that is one problem.

But there is a more fundamental problem: I don't see why there should be any warning for this reform because the reform values of the standard deduction are well above the "default" (that is, current-law) values. So, perhaps this indicates a bug in Tax-Calculator.

In order to make progress on resolving this issue, we need to see the user_mods dictionary submitted to the dropq.reform_warnings_errors function as well as what was returned from that function call. @hdoupe, can you provide that information? I'm not saying we need to fix this before releasing TaxBrain 1.0.3, but we do need to fix this in 1.0.4.

@MattHJensen @GoFroggyRun

@martinholmer
Copy link
Contributor Author

More evidence about the problem reported here in issue 638.

After doing conda install -c ospc taxcalc=0.10.2, I run the following script:

from taxcalc import reform_warnings_errors

user_mods = {
    'policy': {
        2013: {
            '_STD': [[50000, 50000, 50000, 50000, 50000]]
        }
    },
    'consumption': {},
    'behavior': {},
    'growdiff_baseline': {},
    'growdiff_response': {}
}

msg = reform_warnings_errors(user_mods)

assert isinstance(msg, dict)

print('<{}>'.format(msg['warnings']))
print('<{}>'.format(msg['errors']))

I run the script outside the Tax-Calculator repository like this:

iMac2:~ mrh$ python warning_test.py
<>
<>
iMac2:~ mrh$ 

So, there are no warnings and no errors for this policy reform.

This suggests that the problem reported here may indicate a need to improve the TaxBrain code.

@MattHJensen @hdoupe @GoFroggyRun

@martinholmer martinholmer changed the title TaxBrain crash with routine tax reform analysis Unexpected TaxBrain warnings when using valid reform Oct 4, 2017
@martinholmer martinholmer changed the title Unexpected TaxBrain warnings when using valid reform Unexpected TaxBrain warning when using valid reform Oct 4, 2017
@hdoupe
Copy link
Collaborator

hdoupe commented Oct 4, 2017

@martinholmer Thank you for presenting this evidence.

TaxBrain only reads the parameters for single, joint, separate, and head of household. So, the script should be more like:

import taxcalc
import json

reform = {
    'policy': {
               '_STD_headhousehold': {'2013': [50000.0]},
               '_STD_joint': {'2013': [50000.0]},
               '_STD_separate': {'2013': [50000.0]},
               '_STD_single': {'2013': [50000.0]}
          }
}

reform = taxcalc.Calculator.read_json_param_files(json.dumps(reform), None, arrays_not_lists=False)

ew = taxcalc.dropq.reform_warnings_errors(reform)

print(ew)

Then when you run the script, you get:

(aei_dropq) HDoupe-MacBook-Pro:webapp-public henrydoupe$ python errors_warnings_test.py 
{'errors': '', 'warnings': 'WARNING: 2014 _STD_4 value 12380.56 < min value 12400.0\n'}
(aei_dropq) HDoupe-MacBook-Pro:webapp-public henrydoupe$ 

But, STD_4 corresponds to the widow parameter, STD_widow, which TaxBrain does not show. Since TaxBrain does not supply the widow parameter, the default is filled in by the Tax-Calculator. So the _STD parameters that are sent to the Tax-Calculator are:

u'_STD': [[50000.0, 50000.0, 50000.0, 50000.0, 12200.0]]

However, I'm not sure why this last parameter causes an error.

@martinholmer
Copy link
Contributor Author

@hdoupe, Thanks for your explanation of the problem in this comment to this issue. You conclude by saying this:

Since TaxBrain does not supply [that is, does not allow the user to change] the widow parameter, the default is filled in by [TaxBrain using default values from] the Tax-Calculator. So the [2013 values of the] _STD parameters that are sent to the Tax-Calculator are:

u'_STD': [[50000.0, 50000.0, 50000.0, 50000.0, 12200.0]]

However, I'm not sure why this last parameter causes an error.

It doesn't cause an error, it causes a warning in 2014.

The warning is because Tax-Calculator indexed the 2013 value of 12200 to a 2014 value of 12380.56, which is slightly less than the 2014 "default" value of 12400 in the current_law_policy.json file.

Tax-Calculator has never attempted to replicate the rounding rules for indexed parameters, but my guess is that the standard deduction rounding rule is to round the exact indexed value to the nearest hundred dollars. That guess is based on the known values for 2013-2017 in the current_law_policy.json file:

        "value": [[6100, 12200, 6100, 8950, 12200],
                  [6200, 12400, 6200, 9100, 12400],
                  [6300, 12600, 6300, 9250, 12600],
                  [6300, 12600, 6300, 9300, 12600],
                  [6350, 12700, 6350, 9350, 12700]],

If Tax-Calculator were faithfully simulating the rounding rules, then the exact 2014 value of 12380.56 would be rounded to 12400, and there would be no warning. But it doesn't do the rounding, and so the current version of Tax-Calculator issues a 'reform value is less than the "default" value' warning.

Does my analysis of the situation make sense to you?

If so, the question is what to do about it.

One approach is to say that TaxBrain should not be hiding policy parameters from users. That would be, in my view, a sensible statement. But complying with that view would mean adding a Widow box for each of the policy parameters that is indexed by filing-unit-type. My count in the current_law_policy.json file implies adding as many as fifty Widow boxes, but probably fewer because not all parameters in the current_law_policy.json file are shown on the TaxBrain GUI input page.

This approach would eliminate the mystery of not seeing a warning/error message (because it was for a parameter that TaxBrain has, for some reason, chosen not to allow the user to change). And it would allow more accurate specification of reforms (because now every reform that changes a filing-unit-indexed parameter is incompletely, and therefore, incorrectly specified when using TaxBrain.

Another approach is to see if the comparison of reform parameter values with "default" values in Tax-Calculator can be turned off during the years for which the parameter values are "known". That would be a way of avoiding problems caused by the no-rounding inflation-indexing in Tax-Calculator. I'm happy to look into this, but need to know before I start work whether or not this change should be incorporated in a Tax-Calculator 0.10.3 release for "immediate" use by TaxBrain 1.0.3 or whether I should incorporate this change as a bug fix in the pending 0.12.0 release.

My view is that these two approaches are not mutually exclusive.
I think both approaches should be followed.

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

@hdoupe
Copy link
Collaborator

hdoupe commented Oct 5, 2017

@martinholmer 1.0.3 is ready to go. We can incorporate this fix into the next respective PolicyBrain and Tax-Calculator release. It shouldn't affect too many of the runs. I ran the reform in the following years and there were no errors. Further, in most cases, the user should be able to run the reform again and there will be no problems except some (hopefully) minor confusion.

@martinholmer
Copy link
Contributor Author

TaxBrain issue #638 has been resolved by the merge of Tax-Calculator pull request 1578.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants