Gui input processing

[hdoupe]

This PR addreses issue #619. Thanks to @martinholmer's excellent work on the Tax-Calculator reform processing functions (Tax-Calcuator/PR#1502, Tax-Calculator/PR#1503, Tax-Calcuator/PR#1505), the TaxBrain GUI variables can easily be mapped to the Tax-Calculator variables. This simplifies the processing of these parameters in TaxBrain considerably.

In this PR, I operated under the assumption that almost all of the processing of the parameters would be done in the Tax-Calculator. So, the only validation that is done is to make sure that no extra parameters are posted and that only valid data types are accepted (either floating point, integers, or boolean).

The first commit in this PR moves to the new parameter processing methods but makes the minimum amount of changes required to do this. In the second commit, some of the no longer needed code is removed. The third commit adds some comments.

[EDIT: I had to do some back tracking. The first commit edits the personal_results function so that all of the parameter processing is done there. The second commit moves the function converting the fields parameters to json reform style parameters to helpers.py. This function is named to_json_reform and is called from the process_model function. The third commit pins the webapp to Tax-Calculator version 0.10.0.]

TODO List:

• Move minimal_processing function to forms.py and save parameter values for user-edit functionality
• Add processing of checkbox parameters (ID_c, ID_c_medical, ALD_InvInc_ec_base_RyanBrady, etc)
• Audit some of the back-end code to see what is still needed
• Update tests
• Handle errors

@brittainhard @MattHJensen

[brittainhard]

This file seems specialized to your development environment -- makes assumptions on paths. Also, not everyone who is working on webapp is also going to be working on ogusa / taxcalc / btax. I think it would be best to leave this out.

[hdoupe]

Ok that's fine with me. I found it to be useful while I was debugging, but I see your point.

[brittainhard]

@hdoupe I have a couple more comments. Unless i'm mistaken, the only view that uses process_model is the personal_results view. This means that the function is only being used in one place. In that regard, it might be a good idea to move the logic from process_model into the personal results view itself.

Also, since personal_results and file_input are now using identical methods for submitting to the backend, it might be worthwhile to factor out this form submit logic into another function, to reduce the amount of duplicated code.

Does that seem reasonable?

[brittainhard]

Feel free to go ahead and delete this code.

[hdoupe]

Ok, will do. Thanks

[hdoupe]

@brittainhard said

@hdoupe I have a couple more comments. Unless i'm mistaken, the only view that uses process_model is the personal_results view. This means that the function is only being used in one place. In that regard, it might be a good idea to move the logic from process_model into the personal results view itself.

Also, since personal_results and file_input are now using identical methods for submitting to the backend, it might be worthwhile to factor out this form submit logic into another function, to reduce the amount of duplicated code.

Does that seem reasonable?

Both of these suggestions sound reasonable. I'll make these changes by the end of tomorrow. Thanks for reviewing.

[hdoupe]

After this PR, the Webapp-public will require Tax-Calculator Version >= 0.10.0. Should we pin the webapp to 0.10.0 in a separate PR instead of lumping that in with the other changes?

@brittainhard @martinholmer @MattHJensen

[brittainhard]

@hdoupe since this PR needs that version, its probably best to specify it here so you can get all the tests working before we merge it.

[hdoupe]

The most recent commits refactor views.py so that personal_results and file_input use the same function, submit_reforms, to submit reforms. Now, I am working on error-handling logic.

[martinholmer]

@hdoupe said in pull request #641:

The most recent commits refactor views.py so that personal_results and file_input use the same function, submit_reforms, to submit reforms. Now, I am working on error-handling logic.

Thanks for all the work on implementing #619. Hope the rest of your work on #641 goes smoothly.

[brittainhard]

@hdoupe this is looking good. Let me know when you are ready for me to review it again.

[hdoupe]

Thanks @brittainhard and @martinholmer

I should have a first pass of the error handling logic up by the end of the day. @brittainhard I will ping you when that's up.

[hdoupe]

The error handling logic still needs some work. I ran into problems mapping the parameter supplied in the error message from taxcalc to the TaxBrain parameter (e.g. _STD to _STD_2).

My solution was to find the TaxBrain parameter that is either exactly equal to the taxcalc parameter (no problems here) or to find the TaxBrain parameter that starts with the taxcalc parameter and has the same value for that year (e.g. _STD, val. 10,000 maps to _STD_2, val. 10,000). This is fine as long as the errant value is different from the other values that start with the taxcalc parameter. For example, this works fine:

and works fine here:

But, this does not:

[EDIT for clarity]
The error message for the third case is:
WARNING: 2017 _STD value 10000.0 < min value 12700.0

The third case fails because we are looking for a parameter-value pair ("_STD", 10000.0) Any of the following pairs will work ("_STD_0", 10000.0), ("_STD_1", 10000.0), ("_STD_2", 10000.0), ("_STD_3", 10000.0) since each parameter name starts with "_STD" and has a value 10000.0. Thus, the error could be displayed by any one of the fields depending on which pair comes up first in the search. Here the first but wrong match is ("_STD_3", 10000.0) and the correct match is ("_STD_1", 10000.0). Given the information that I currently have I don't think I can locate the correct match in all cases.

The solution that I have in mind is to specify the taxcalc parameter name and the index (or name e.g. "single") of the errant value (if that parameter is a list) in the error message. This would require modifying the Tax-Calculator code. Does anyone have any other ideas?

The other problem is that if a parameter is initially set within the correct range but inflates out of that range, then an error would not be thrown. This is not desired behavior. Should warning and error messages be shown for all years where the value of the parameter is out of range?

Or, given that the value of the parameter is ok initially, should a warning message be shown for just the first year where the value is out of range? The logic here would be to show error/warning message if:

1. the user sets an errant value and warning is only for the first year in which the value is errant and not the remaining years of the budget window
2. the user sets a valid value, but that value inflates out of the acceptable range. then show warning for the first year where the value is errant but not the remaining years of the budget window

What does everybody think here? I'll take another look at how the error/warning logic worked before, but this may be a good time for everyone to revisit how the errors/warnings worked before and either affirm this behavior or raise objections.

cc @brittainhard @martinholmer @MattHJensen

[martinholmer]

@hdoupe said:

The error handling logic still needs some work. I ran into problems mapping the parameter supplied in the error message from taxcalc to the TaxBrain parameter (e.g. _STD to _STD_2).

My solution was to find the TaxBrain parameter that is either exactly equal to the taxcalc parameter (no problems here) or to find the TaxBrain parameter that starts with the taxcalc parameter and has the same value for that year (e.g. _STD, val. 10,000 maps to _STD_2, val. 10,000). This is fine as long as the errant value is different from the other values that start with the taxcalc parameter.

Then @hdoupe show two Standard Deduction examples that work fine and a third Standard Deduction example that does not.

@hdoupe, Maybe I'm slow on the uptake this morning, but can you explain more about why your approach does not work in the third example?

[hdoupe]

@martinholmer Sorry for not making that clear enough. I updated my initial comment in an effort to clear things up.

[martinholmer]

@hdoupe said:

The error message for the third case is:
WARNING: 2017 _STD value 10000.0 < min value 12700.0

The third case fails because we are looking for a parameter-value pair ("_STD", 10000.0) Any of the following pairs will work ("_STD_0", 10000.0), ("_STD_1", 10000.0), ("_STD_2", 10000.0), ("_STD_3", 10000.0) since each parameter name starts with "_STD" and has a value 10000.0. Thus, the error could be displayed by any one of the fields depending on which pair comes up first in the search. Here the first but wrong match is ("_STD_3", 10000.0) and the correct match is ("_STD_1", 10000.0). Given the information that I currently have I don't think I can locate the correct match in all cases.

The solution that I have in mind is to specify the taxcalc parameter name and the index (or name e.g. "single") of the errant value (if that parameter is a list) in the error message. This would require modifying the Tax-Calculator code. Does anyone have any other ideas?

Have you considered adding to TaxBrain a dictionary that maps non-scalar parameters from the TaxBrain name to the Tax-Calculator name? If I'm not mistaken, your job would be easy if you had the inverse of the JSON_REFORM_SUFFIXES dictionary in the policy.py file.

The dictionary might look something like this:

{
    'mars_indexed': {
        '0': 'single',
        '1': 'joint',
        '2': 'separate',
        '3': 'headhousehold'},
    'eic_indexed': {...},
    'ided_indexed': {...}
}

Would this dictionary work? If you think it would, I would be happy to add it to the policy.py so that it would be easier to synchronize this new one with the existing JSON_REFORM_SUFFIXES dictionary.

[hdoupe]

@martinholmer That would help. But, I think the issue is that for parameters with a list of values such as _STD the offending value cannot be identified with the error message: WARNING: 2017 _STD value 10000.0 < min value 12700.0. If the error message was : WARNING: 2017 _STD_1 value 10000.0 < min value 12700.0 then the dictionary that you mentioned above could be used to map back to STD_joint. Does that make sense?

[martinholmer]

@hdoupe said:

That would help. But, I think the issue is that for parameters with a list of values such as _STD the offending value cannot be identified with the error message: WARNING: 2017 _STD value 10000.0 < min value 12700.0. If the error message was : WARNING: 2017 _STD_1 value 10000.0 < min value 12700.0 then the dictionary that you mentioned above could be used to map back to STD_joint. Does that make sense?

Yes, thanks for being patient with me. I'm a little slow today. Let me think about this and prepare a Tax-Calculator pull request for you to review to see if it will meet your TaxBrain needs. OK?

Wait. I'm still thinking too slow. Is the following the correct way to think about this issue?

Our problem is that the error/warning messages have to make sense to both Tax-Calculator users (who know about the parameter names in current_law_policy.json) and TaxBrain issues who expect to see the error/warning message below the parameter's input box. So, the message (which is shown unedited to Tax-Calculator users) needs to use parameter names from current_law_policy.json. But (and this is where your problem arises) the message needs to have enough information to allow TaxBrain to display a translated or edited message below the correct box.

[hdoupe]

@martinholmer said

But (and this is where your problem arises) the message needs to have enough information to allow TaxBrain to display a translated or edited message below the correct box.

Yes, this is the problem. The unedited messages may cause some problems when shown to Tax-Calculator users since they may try the same reform above with 'STD' = [[10000, 10000, 10000, 10000, 10000]]. But, they would not be able to identify (unless they perform some some detective work) that the issue arises from the second entry in this list. So it may be useful to display the error message WARNING: 2017 _STD_single value 10000.0 < min value 12700.0. That way the user immediately knows which parameter value is causing the problem. That would also give me enough info to display the error/warning under the field in TaxBrain.

[hdoupe]

@martinholmer said

Let me think about this and prepare a Tax-Calculator pull request for you to review to see if it will meet your TaxBrain needs. OK?

Thanks for your consideration and your working on the problem

[hdoupe]

@brittainhard It works with Taxbrain and the dynamic behavioral page. I think it would work with the dynamic_elasticities too. But, I think OG-USA would need a Parameters.default_data type function before this would work.

[martinholmer]

In pull request #641, @hdoupe and @brittainhard discussed PB code like this:

     ID_c_3 = CommaSeparatedField(default=None, blank=True, null=True)
     ID_c_4 = CommaSeparatedField(default=None, blank=True, null=True)

saying things like this:

I was working with a local version of Tax-Calculator, which had these new parameters, that I thought was equivalent to 0.10.1. So, I made some changes to the PolicyBrain to accommodate these new parameters. Then I realized that they haven't been included in a taxcalc package yet. I figured that they would be added eventually so I just commented the code instead of deleting it.

Parameters names like ID_c_3 will NEVER be included in Tax-Calculator, but JSON reform files can include the equivalent ID_c_headhousehold parameter name.

If there is any confusion about this, we should clear it up immediately.

[hdoupe]

@martinholmer Right, we were referring to their equivalent representation in TaxBrain.

[brittainhard]

@hdoupe this looks good to me, let me know when you are ready for me to merge it.

[hdoupe]

@brittainhard Thanks for the review. This is ready to merge.