New growth rate handles #383

Amy-Xu · 2015-10-11T22:50:34Z

In this PR, all the growth rate handles are refactored as the new class style in #367. In terms of functionality, this PR is equivalent to #368, which can be closed after this PR's open. Similar to the Behavior class, this Growth class would copy both the baseline and reform calculators and apply user-specified rate on both calculators. A quick demo in a notebook is as follows:

This is an example changing the general economic growth rate to 4%, in real terms, for both baseline and reform (add surtax on Itemized Deduction Benefits).

Without changing the growth rates:
Baseline:

Reform:

Increase general economic growth to 4% a year
Baseline:

Reform:

Haven't been able to change the algebra of growth rates for webapp yet.

MattHJensen · 2015-10-12T13:59:03Z

@Amy-Xu, sounds like calling these things during increment_year() are fine. Just on the phone with @talumbau. He might provide some more detail soon.

talumbau · 2015-10-12T20:05:57Z

taxcalc/calculate.py

+        if isinstance(growth, Growth):
+            self.growth = growth
+        else:
+            self.growth = Growth(start_year=policy.start_year)


Hmm.. this idiom seems bad to me (used for both Growth and Behavior). If someone passes a Growth object for growth, then we set it to self.growth. If they pass anything else (a string, some other kind of object, whatever), we just silently make a new Growth object and assign it to self.growth. Basically the user could assume he/she is sending in an object for growth but actually ending up with something else (with no error/warning message). That seems bad. How about something like this?

if growth: if isinstance(growth, Growth): self.growth = growth else: raise ValueError("Must supply growth as a Growth object") else: self.growth = Growth(start_year=policy.start_year

Amy-Xu · 2015-10-12T20:40:20Z

@talumbau Thanks! Will fix those very soon. I got very confused that Travis CI keeps telling me it cannot find that growth.json even thought all my local tests are passing. Could you take a glance of that error?

MattHJensen · 2015-10-12T21:19:52Z

@talumbau Thanks! Will fix those very soon. I got very confused that Travis CI keeps telling me it cannot find that growth.json even thought all my local tests are passing. Could you take a glance of that error?

Try adding growth.json to MANIFEST.in

talumbau · 2015-10-12T21:30:52Z

yes, I agree with @MattHJensen, I believe that adding it to the MANIFEST.in will put it in the egg file so that it can be read when running via Travis CI.

MattHJensen · 2015-10-13T15:13:14Z

taxcalc/growth.json

@@ -0,0 +1,26 @@
+{


@Amy-Xu could you take a crack at filling these in? Here is a suggestion, although please feel free to modify these.

long_name: "Deviation from CBO forecast of baseline economic growth (percentage point)"

description: "The data underlying this model are extrapolated to roughly match the CBO's projection of the economy's development over the 10-year federal budget window, with each type of economic data extrapolated at a different growth rate. This parameter allows a factor to be subtracted or added to those growth rates for the construction of the economic baseline. For example if you supply .02 (or 2%), then 0.02 will be added to the wage and salary growth rate, interest income growth rate, dividend growth rate, schedule E income growth rate, and all other growth rates used to extrapolate the underlying dataset."

long_name: "Replacement for CBO real GDP growth in economic baseline (percent)".

description: "The data underlying this model are extrapolated to roughly match the CBO's projection of the economy's development over the 10-year federal budget window, with each type of economic data extrapolated at a different growth rate. One of the growth rates taken from the CBO is GDP growth. This parameter allows you to specify a real GDP growth rate, and all other rates will be modified to maintain the distance between them and GDP growth in the CBO baseline. For example, if the CBO growth rate for one year is 0.02 and the user enters 0.018 for this parameter, then 0.002 will be subtracted from every growth rate in the construction of the economic baseline, including wage and salary growth, interest income growth, dividend growth, and many others.

cc @feenberg @martinholmer

Amy-Xu · 2015-10-13T21:19:04Z

I changed the application of adjustment and targets. More detailed description can be found in growth.json description node. Here are the tables after applying adjustments or targets. Default results remain the same.

GDP growth (real terms) at 4%:
Baseline:

User-specified Plan (add 0.02 Itemized Deduction Benefit Surtax):

General economic growth rate increase by 1%:
Baseline:

User specified plan:

@MattHJensen Could you take a look at the tables? I will add tests for this branch soon.

Amy-Xu · 2015-10-14T00:31:28Z

@MattHJensen @talumbau just added tests for this PR. Should have finished everything I have in mind. Could you review? Thanks!

talumbau · 2015-10-14T01:54:14Z

Here are the questions that I have:

Does it make sense to specify both a factor target and a factor adjustment? In my conversation with Matt, my understanding is that it does not make sense. Should we check if both are specified and raise an exception if that occurs?
Is it the case that the following user_mods dictionaries should produce all the same results:
- user_mods = {}
- user_mods = {2015: {'_factor_target': [[0.0]]}}
- user_mods = {2015: {'_factor_adjustment': [[0.0]]}}
  (this is relevant for certain ways a user could interact with the webapp)
following the maxim of "make the common case fast", would it be OK to implement increment_year as follows:

    def increment_year(self):
        if self.growth.factor_adjustment != 0.0:
            adjustment(self, self.growth.factor_adjustment,
                       self.policy.current_year + 1)
        if np.any(self.growth._factor_target != 0.0):
            target(self, self.growth._factor_target,
                   self.policy._inflation_rates,
                   self.policy.current_year + 1)
        self.records.increment_year()
        self.policy.set_year(self.policy.current_year + 1)
        self.behavior.set_year(self.policy.current_year)

MattHJensen · 2015-10-14T02:18:52Z

@Amy-Xu, quick question/comment on issue 1 from @talumbau above. Leaving replies on everything to you.

Does it make sense to specify both a factor target and a factor adjustment? In my conversation with Matt, my understanding is that it does not make sense. Should we check if both are specified and raise an exception if that occurs?

Would it make sense to do this through the validations attribute? As to raising an exception in taxcalc, we might want to do this something more general and raise an exception in taxcalc when any validation rule is violated.

cc @zrisher

Amy-Xu · 2015-10-14T02:54:35Z

@talumbau for the first question, yes we should check whether both are specified at the same time. But as @MattHJensen just pointed out here, I'm not quite sure where's the best place to implement this check. I can definitely do it in this PR if necessary.

Q2. Yes, if in each case that's the only thing users specified in user_mods.

Q3. Yes, that would make the common case fastest. And if I move those two condition from where they are right now to this new increment_year, those growth-rate cases wouldn't be slowed down either. I guess I can just go ahead and make the changes in this PR?

talumbau · 2015-10-14T12:01:38Z

Q3. Yes, that would make the common case fastest. And if I move those two condition from where they are right now to this new increment_year, those growth-rate cases wouldn't be slowed down either. I guess I can just go ahead and make the changes in this PR?

Sure I think it would make sense to make the changes in this PR. On my end, I am working on getting the webapp ready for these additional inputs.

talumbau · 2015-10-14T12:29:23Z

taxcalc/growth.json

@@ -0,0 +1,24 @@
+{
+    "_factor_adjustment":{
+        "long_name": "Deviation from CBO forecast of baseline economic growth (percentage point",


edit: "(percentage point" -> "(percentage point)"

actually why not just "(percent)" like in _factor_target

Good point. I'm not sure 🌴 @MattHJensen?

the _factor_target allows the user to specify a GDP growth rate as a percent. The _factor_adjustment allows the user to specify a percentage point deviation from existing rates (If a existing growth rate is 10%, a 10 percent increase makes it 11%, a 10 percentage point increase makes it 20%.)

got it. thanks

Amy-Xu · 2015-10-14T14:11:08Z

@talumbau @MattHJensen It doesn't seem the validation for these two parameters fits the min/max format I proposed a while ago. So I guess the easiest way to add validation would just be throwing a value error if the two parameters are non-zero at the same time. We can probably think of more general solutions later on. What do you think?

talumbau · 2015-10-14T15:33:30Z

I would be a proponent of "ugly exception raising now, clean generalized validation behavior later" but that is just me.

Amy-Xu · 2015-10-14T16:54:56Z

@MattHJensen Then I guess this PR should be good now.

MattHJensen · 2015-10-14T17:00:28Z

Thanks Amy. Merging.

New growth rate handles

Amy-Xu added 5 commits October 11, 2015 14:56

added growth class and adjust weights

a1cae0b

pep8

4810352

pep8 again

99e2703

pep8

5be1fa2

typo

c12d12e

Amy-Xu added 3 commits October 12, 2015 15:13

removed the growth function

92fd174

merged with upstream master

23242ee

imported growth in tests

37dd2f2

talumbau reviewed Oct 12, 2015
View reviewed changes

import & warnings

c82fab8

added growth.json to MANIFEST

1af0f21

MattHJensen reviewed Oct 13, 2015
View reviewed changes

MattHJensen mentioned this pull request Oct 13, 2015

[WIP]Adjust economic growth rates #368

Closed

Amy-Xu added 3 commits October 13, 2015 16:59

changed rates application

e18d403

pep8

d30853f

added long names and descriptions

0fefa3d

talumbau mentioned this pull request Oct 13, 2015

New economic baseline section. ospc-org/ospc.org#61

Closed

added tests for growth

9ff134a

Amy-Xu changed the title ~~[WIP]New growth rate handles~~ New growth rate handles Oct 14, 2015

talumbau reviewed Oct 14, 2015
View reviewed changes

Amy-Xu added 3 commits October 14, 2015 09:44

make common case fast

5081114

raise value error if both paramters are zeros

3e76343

typo

d2d3753

MattHJensen added a commit that referenced this pull request Oct 14, 2015

Merge pull request #383 from Amy-Xu/new_growth

2e7e7f3

New growth rate handles

MattHJensen merged commit 2e7e7f3 into PSLmodels:master Oct 14, 2015

Amy-Xu deleted the new_growth branch November 19, 2015 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New growth rate handles #383

New growth rate handles #383

Amy-Xu commented Oct 11, 2015

MattHJensen commented Oct 12, 2015

talumbau Oct 12, 2015

Amy-Xu commented Oct 12, 2015

MattHJensen commented Oct 12, 2015

talumbau commented Oct 12, 2015

MattHJensen Oct 13, 2015

Amy-Xu commented Oct 13, 2015

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

MattHJensen commented Oct 14, 2015

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

talumbau Oct 14, 2015

talumbau Oct 14, 2015

Amy-Xu Oct 14, 2015

MattHJensen Oct 14, 2015

talumbau Oct 14, 2015

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

Amy-Xu commented Oct 14, 2015

MattHJensen commented Oct 14, 2015

New growth rate handles #383

New growth rate handles #383

Conversation

Amy-Xu commented Oct 11, 2015

MattHJensen commented Oct 12, 2015

talumbau Oct 12, 2015

Choose a reason for hiding this comment

Amy-Xu commented Oct 12, 2015

MattHJensen commented Oct 12, 2015

talumbau commented Oct 12, 2015

MattHJensen Oct 13, 2015

Choose a reason for hiding this comment

Amy-Xu commented Oct 13, 2015

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

MattHJensen commented Oct 14, 2015

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

talumbau Oct 14, 2015

Choose a reason for hiding this comment

talumbau Oct 14, 2015

Choose a reason for hiding this comment

Amy-Xu Oct 14, 2015

Choose a reason for hiding this comment

MattHJensen Oct 14, 2015

Choose a reason for hiding this comment

talumbau Oct 14, 2015

Choose a reason for hiding this comment

Amy-Xu commented Oct 14, 2015

talumbau commented Oct 14, 2015

Amy-Xu commented Oct 14, 2015

MattHJensen commented Oct 14, 2015