Add validity checking for non-behavior assumption parameters #1992
Conversation
martinholmer
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1992 +/- ##
======================================
Coverage 100% 100%
======================================
Files 38 38
Lines 3756 3808 +52
======================================
+ Hits 3756 3808 +52
Continue to review full report at Codecov.
|
| # raise error if specifying both behavior and growdiff_response | ||
| if behv_dict and gdiff_resp_dict: | ||
| msg = 'both behavior and growdiff_response are specified' | ||
| raise ValueError('ERROR: ' + msg + '\n') |
There was a problem hiding this comment.
@martinholmer do you think this type of integrated test is a rare situation? That is, do you think there will be more tests or validation checks that need to know information about the parameters specified for different TC classes (Policy, Behavior, Consumption, etc.)?
There was a problem hiding this comment.
No, I don't think it is that rare a situation. This seems to be what came up in Tax-Calculator issue #1981. There will be a few more when I add the Growthmodel class and its parameters in the Growmod class.
Seems as if TaxBrain needs to put its call to Calculator.read_json_param_objects in a try statement.
There was a problem hiding this comment.
No, I don't think it is that rare a situation. This seems to be what came up in Tax-Calculator issue #1981. There will be a few more when I add the Growthmodel class and its parameters in the Growmod class.
Ok, thanks.
Seems as if TaxBrain needs to put its call to Calculator.read_json_param_objects in a try statement.
Sure, that would work.
What do you think about adding a method to the Calculator class that does a validation test before increment_year is called or on initialization? That way, the read_json_params_objects focuses on reading the JSON files and the validation is mostly done elsewhere. This method could look something like:
@staticmethod
def integrated_validation(pol, behv, growdiff_resp, growdiff_base):
assert not (growdiff_resp.has_any_response() and behv.has_any_response())
... do some more tests on the objects here
Note: you can find a similar check in tbi.tbi_utils.calculate:
# always prevent both behavioral response and growdiff response
if behv.has_any_response() and growdiff_response.has_any_response():
msg = 'BOTH behavior AND growdiff_response HAVE RESPONSE'
raise ValueError(msg)
There was a problem hiding this comment.
@hdoupe said:
What do you think about adding a method to the Calculator class that does a validation test before
increment_yearis called or on initialization? That way, theread_json_params_objects[function] focuses on reading the JSON files and the validation is mostly done elsewhere.
That was my first thought, but the Calculator object does not include any growdiff information.
I couldn't see an easy way around that problem. Did I miss an easy solution?
There was a problem hiding this comment.
Ahh, I see. So, the Growdiff information is applied to Growfactors, which is used by Policy and Records. Then, by the time, the Calculator class is initialized, the growdiff information is impossible to access. Is that right?
There was a problem hiding this comment.
Well, I have one idea. What do you think about propagating the Growdiff.has_any_response up to Growfactors when the Growdiff.apply_to method is called? Then, it could be accessed from self.__policy._gfactors.has_growdiff_response. It's not the most elegant solution but it could work.
Plus, integrated_validation could be called from reform_warnings_errors since Growfactors, Policy, Consump, and Behavior are all available.
This approach could look like:
# growdiff class
def update(self, name, year, diff):
"""
Add to self.gfdf[name][year] the specified diff amount.
"""
if self.used:
msg = 'cannot update growfactors after they have been used'
raise ValueError(msg)
if diff != 0: # logically equivalent to Growdiff.has_any_response
self.has_growdiff_response = True
self.gfdf[name][year] += diff
# Calculator class
def calc_all(self, zero_out_calc_vars=False):
"""
Call all tax-calculation functions for the current_year.
"""
Calculator.integrated_validation(self.__policy, self.__behavior, self.__consumption,
self.__policy._gfactors)
# conducts static analysis of Calculator object for current_year
...
@staticmethod
def integrated_validation(pol, behv, consump, gfactors):
assert not (gfactors.has_growdiff_response
and behv.has_any_response())
There was a problem hiding this comment.
@martinholmer what was your expensive approach? Or, was this it?
There was a problem hiding this comment.
@hdoupe said:
What do you think about propagating the
Growdiff.has_any_responseup toGrowfactorswhen theGrowdiff.apply_tomethod is called? Then, it could be accessed fromself.__policy._gfactors.has_growdiff_response.
I see two sorts of problems with this approach:
-
It defers the parameter validity checking until the calculation stage. I thought the whole point of the parameter validity checking capability we have built for TaxBrain is to catch problematic parameter values before calculation starts.
-
It relies on the user calling the two
apply_tocalls correctly and differently. It is only thegrowdiff_response.apply_tocall that needs "propagating"; not thegrowdiff_baseline.apply_tocall. If a user is knowledgeable enough to do this correctly, then that user doesn't need the parameter validity checking. It's only the clueless Python programmers that need to help and I don't think we can rely on them to make the twoapply_tocalls correctly.
The only foolproof way to do this (as far as I can see) is to make the growdiff_baseline and growdiff_response objects two different classes. But, as I said before, that's a lot of work.
There was a problem hiding this comment.
Ok, I see. That was the only idea I had. I'll move the read_json_param_objects call into a try block.
There was a problem hiding this comment.
Thank you for providing careful and clear explanation for why this approach wouldn't work so well.
|
@martinholmer This PR looks good to me. |
|
@hdoupe, Note that pull request #1998 removes the aspect of pull request #1992 that created the possibility that the |
|
Great, thanks for the update @martinholmer. |
This pull request moves the two
Behavior._validate_*methods to the ParametersBase class and then uses them to add validity checking for consumption, growdiff_baseline, and growdiff_response parameters.