Add logic to enforce policy parameter 'validations'

[martinholmer]

Some parameters in the current_law_policy.json file contain a field called validations that contains min and/or max constraints that must be satisfied when the value of the parameter is changed in a reform. Tax-Calculator has never enforced these min/max validations rules. This pull request adds logic that enforces these rules. It also adds two new tests. One test checks the content of the JSON validations fields and that the new Policy.VALIDATED_PARAMETERS set is accurate. The other new test ensures that code coverage is maintained by triggering a min-rule violation and a max-rule violation.

Interestingly, the new code revealed two problems that are also fixed in this pull request. The first was that the validations for the _STD_Aged parameters was too restrictive in that it triggered a fatal error when implementing the reform specified in the RyanBrady.json file. The second was that reform 25 in the test_reforms.py file triggered a fatal error.

One implication of this Tax-Calculator enhancement is that there is no need to do validations checking in TaxBrain. An unchecked reform can be passed to Tax-Calculator and an error message describing the nature of the validations error will be passed back as a ValueError to TaxBrain where is can be shown to the TaxBrain user. If the nature of the ValueError message needs to be modified in any way to facilitate this elimination of code duplication between TaxBrain and Tax-Calculator, that should be an easy task.

@MattHJensen @feenberg @Amy-Xu @andersonfrailey @hdoupe @GoFroggyRun @codykallen
@PeterDSteinberg @brittainhard

[codecov-io]

Codecov Report

Merging #1502 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1502      +/-   ##
==========================================
+ Coverage   99.91%   99.92%   +<.01%     
==========================================
  Files          37       37              
  Lines        2484     2507      +23     
==========================================
+ Hits         2482     2505      +23     
  Misses          2        2

Impacted Files	Coverage Δ
taxcalc/policy.py	98.85% <100%> (+0.41%)	⬆️

---

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 3eca10f...34be135. Read the comment docs.

[codykallen]

@martinholmer mentioned

The first was that the validations for the _STD_Aged parameters was too restrictive in that it triggered a fatal error when implementing the reform specified in the RyanBrady.json file.

The validation for this is due to the lack of data on non-itemizers. Reducing or eliminating the additional standard deduction without altering other parameters should (in reality) increase the number of itemizers but may not have much effect in Tax Calculator. Perhaps a better validation for this parameter would be to check that the standard deduction plus the additional standard deduction does not decrease under a reform.

[martinholmer]

@codykallen said:

The validation for this is due to the lack of data on non-itemizers. Reducing or eliminating the additional standard deduction without altering other parameters should (in reality) increase the number of itemizers but may not have much effect in Tax Calculator.

Yes, I understand that. But these deduction-related validations are very different from the others that are based in logic (such as bracket1 must be no greater than bracket2). These deduction-related validations are there only because we have never gotten around to imputing itemized-deduction expenses for non-itemizers as @MattHJensen requested a year ago in taxdata issue 32.

@codykallen also said:

Perhaps a better validation for this parameter would be to check that the standard deduction plus the additional standard deduction does not decrease under a reform.

For me, the benefit of this enhancement is not worth its cost. All these deduction-related validations will be eliminated once taxdata issue 32 is resolved. But if you would like to propose code that does this new type of validation checking, feel free to prepare a pull request.

@MattHJensen @feenberg @Amy-Xu @andersonfrailey @hdoupe @GoFroggyRun @codykallen
@PeterDSteinberg @brittainhard

[MattHJensen]

@martinholmer, on TaxBrain, we have allowed users to bypass the validations. It does not appear that this PR will allow that anymore. I think it is better to show warnings rather than errors.

A common scenario where someone would want to bypass the validations is when they reduce the standard deduction while eliminating itemized deductions. The error from not having imputed itemized deductions to non-itemizers would be small in those situations, and I think the user should be able to decide whether to accept that error.

[martinholmer]

@MattHJensen said about #1502:

On TaxBrain, we have allowed users to bypass the validations. It does not appear that this PR will allow that anymore. I think it is better to show warnings rather than errors.

Where is the switch on TaxBrain that allows users to "bypass the validations"?
Or, are you saying that TaxBrain always bypasses the validations?

@MattHJensen continued:

A common scenario where someone would want to bypass the validations is when they reduce the standard deduction while eliminating itemized deductions. The error from not having imputed itemized deductions to non-itemizers would be small in those situations, and I think the user should be able to decide whether to accept that error.

It seems as if all the problems with the validations are rooted in the fact that we have never gotten around to imputing itemized-deduction expenses for non-itemizers as you requested a year ago in taxdata issue 32.

What is the status of taxdata issue 32? Why don't we make that a priority? Because when 32 is implemented, we can drop all the validations related to the standard and itemized deduction parameters.

Or, if the errors caused by not having itemized-expense amount for non-itemizers are often "small", why not drop now all the validations related to the standard and itemized deduction parameters? I'll be happy to prepare a pull request that does that. What do you think?

[martinholmer]

After using TaxBrain to specify parameter values that fail the validations, I see that TaxBrain warns (but in a not-informative way because it does say which parameters are not valid) but then accepts all invalid parameter values when user clicks the "Show Me ..." button a second time.

I consider this TaxBrain behavior to be a bug. Because it allows logically incorrect parameter values to be specified as shown in this sample run:

If we allow this logically inconsistent reform to be simulated, then we should simply remove all the validations. @MattHJensen, do you want me to prepare a pull request that drops all the validations info from the current_law_policy.json file?

[MattHJensen]

I think the ideal approach would be for the user to see a warning and explanation before being allowed to continue with the reform if they choose. This is in line with our long standing goal of allowing our users, wherever possible, to make key modeling decisions for themselves.

I have come across several situations myself where I have chosen to bypass the warning, but I also think it is our responsibility communicate the limitation to our users.

As @martinholmer points out, we don't currently attain the ideal because we don't offer an adequate explanation. I've received this feedback before from Kevin Hassett, but I failed to do anything about it at the time.

One option would be to stick with the warning system (albeit now in Tax-Calculator rather than TaxBrain) and simply make the explanation more informative.

Another option would be to move the warning to the documentation and code comments. My fear is that those would be very easy to overlook.

I think it is too limiting to strictly enforce the rules.