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

Convert DependentCredit_before_CTC from bool to int #782

Merged
merged 8 commits into from
Dec 20, 2017

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Dec 19, 2017

Resolves #765. This PR treats DependentCredit_before_CTC as an integer instead of a boolean parameter which allows the user to very the parameter's value over time.

EDIT:
Old behavior:
screen shot 2017-12-19 at 5 12 11 pm

New behavior:
screen shot 2017-12-19 at 5 12 49 pm

@hdoupe
Copy link
Collaborator Author

hdoupe commented Dec 19, 2017

Are there any other boolean parameters that we would like to convert into comma separated value parameters?

My plan is to only convert the ones where we see an obvious use case for allowing changes over time. If we like the behavior introduced for these select parameters, then we can convert the rest of the boolean parameters. I think this will minimize the number of changes to the database.

cc @MattHJensen @martinholmer @codykallen

@MattHJensen
Copy link
Contributor

MattHJensen commented Dec 19, 2017

@hdoupe, once you feel comfortable with the database changes, I think we should make this change for all boolean parameters, as it is very difficult to anticipate what policymakers will want to delay or sunset.

@martinholmer
Copy link
Contributor

@hdoupe said:

This [PolicyBrain] PR [#782] treats DependentCredit_before_CTC as an integer instead of a boolean parameter which allows the user to very the parameter's value over time.

OK, but this parameter is boolean in Tax-Calculator. So that means the JSON reform file TaxBrain generates must have them as boolean even though they are presented as 0s and 1s in the TaxBrain GUI.

Is there a reason the GUI can't use comma-separated boolean values?

@hdoupe
Copy link
Collaborator Author

hdoupe commented Dec 19, 2017

@MattHJensen Ok, that makes sense. I'll get this on the test app today. If everything goes well, then I will apply this change to the rest of the boolean parameters.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Dec 19, 2017

@martinholmer asked:

OK, but this parameter is boolean in Tax-Calculator. So that means the JSON reform file TaxBrain generates must have them as boolean even though they are presented as 0s and 1s in the TaxBrain GUI.

Is there a reason the GUI can't use comma-separated boolean values?

The GUI can tolerate comma separated boolean values. However, I think that we should also allow the submission of 1/0 since they are logically equivalent.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Dec 20, 2017

@martinholmer I'm getting the following error when I submit this reform using PR #782 :
screen shot 2017-12-20 at 10 05 15 am

A simpler example of this reform is run here:

import taxcalc

policy_dict = {
    u'policy': {
        2019: {
            u'_DependentCredit_before_CTC': [1],
        },
    },
    u'growdiff_response': {},
    u'consumption': {},
    u'behavior': {},
    u'growdiff_baseline': {}
}

ew  = taxcalc.tbi.reform_warnings_errors(policy_dict)

print("errors warnings for: ")
print(policy_dict)
print('\n')
print(ew)

policy_dict["policy"][2019]["_DependentCredit_before_CTC"] = [1.0]

ew = taxcalc.tbi.reform_warnings_errors(policy_dict)
print("\nerrors warnings for: ")
print(policy_dict)
print('\n')
print(ew)

I get the following output:

(aei_dropq) HDoupe-MacBook-Pro:PolicyBrain henrydoupe$ python tc_pr_1775.py 
errors warnings for: 
{u'policy': {2019: {u'_DependentCredit_before_CTC': [1]}}, u'growdiff_response': {}, u'behavior': {}, u'consumption': {}, u'growdiff_baseline': {}}


{'errors': '', 'warnings': ''}

errors warnings for: 
{u'policy': {2019: {u'_DependentCredit_before_CTC': [1.0]}}, u'growdiff_response': {}, u'behavior': {}, u'consumption': {}, u'growdiff_baseline': {}}


{'errors': 'ERROR: 2019 _DependentCredit_before_CTC value 1.0 is not boolean\n', 'warnings': ''}

It seems like 1.0 is not a valid boolean according to Tax-Calculator even though we also have:

In [1]: bool(1.0) == bool(1) == bool(True)
Out[1]: True

PolicyBrain reads the user input from the GUI as strings. The strings with digits are converted to python floats. I could add a special case for the boolean parameters that are entered as 1/0/1.0/0.0 that converts them to 1/0 or True/False. However, I think a better solution would be for Tax-Calculator to accept 1.0 and 0.0 as valid boolean values. @martinholmer what are your thoughts on this?

@hdoupe hdoupe merged commit e2c32a2 into ospc-org:master Dec 20, 2017
@MattHJensen
Copy link
Contributor

Just my 2 cents, I am more eager for TaxBrain to be able to accept True/False than for either TaxBrain or Tax-Calculator to accept 1.0, 1, 0, 0.0. We just have so many numeric inputs, including switches, it refreshing to differentiate switches.

@hdoupe hdoupe mentioned this pull request Dec 20, 2017
@hdoupe
Copy link
Collaborator Author

hdoupe commented Dec 20, 2017

@MattHJensen Thanks for your insight. I moved your comment to issue #786. Sorry, I probably should have left this PR open longer. I was in a bit of a rush to get this merged, get a pre-release up, and move on to #763. I should have waited longer to make sure that we were all on board.

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.

3 participants