fixing t indexing for inputs for cyclical models

[sbenthall]

This is PR partially fixes #1022.

It makes the changes for cyclical models only. This involves:

• rotating the input parameters so that the "last" element, which corresponds to the parameters of the 0th period, goes to the 0th place.
• changing the solver to iterate backwards through these elements properly
• using the t'th LivPrb in mortalilty for this case.

... it should also involve using t not t - 1 when drawing shocks but ... we don't seem to have automated tests for a multi-period cyclical case and so they are not failing.

Because this is a pretty complicated change, I'm going to try to fix it for the cyclical cases first and then do the cycles = 1 cases in a separate PR.

Please ensure your pull request adheres to the following guidelines:

• Tests for new functionality/models or Tests to reproduce the bug-fix in code.
• Updated documentation of features that add new functionality.
• Update CHANGELOG.md with major/minor changes.

[codecov-commenter]

Codecov Report

Merging #1039 (5fce3e2) into master (4f2ef4d) will increase coverage by 0.01%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
+ Coverage   72.24%   72.26%   +0.01%     
==========================================
  Files          66       66              
  Lines        9883     9889       +6     
==========================================
+ Hits         7140     7146       +6     
  Misses       2743     2743              

Impacted Files	Coverage Δ
HARK/core.py	89.70% <75.00%> (+0.02%)	⬆️
HARK/ConsumptionSaving/ConsIndShockModel.py	85.88% <100.00%> (+0.03%)	⬆️
...nsumptionSaving/tests/test_IndShockConsumerType.py	100.00% <100.00%> (ø)

---

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 4f2ef4d...5fce3e2. Read the comment docs.

[sbenthall]

It looks like the multi-period cyclical model input order is not documented anywhere, so no changes are needed to accommodate this.

[Mv77]

My only concern comes from my unfamiliarity with cyclical models in HARK.

For a finite horizon, non-cyclical model with two periods, I think HARK requires len(PermGroFac) == 1. The single element in the list is the growth factor of permanent income between the first and second periods. The terminal period is, in this sense, "left out" from the list which has length 1 for a 2-period model.

How does this logic change when I'm considering an infinite-horizon model with a cycle of two periods? I know Chris, Matt and you had some lengthy discussion on how to treat the "terminal" period in cyclical vs non-cyclical models, which I did not follow, but I wanted to make sure this change (which looks good to me) is consistent with whatever you settled on.

[sbenthall]

My understanding is that when cycles == 0, it's infinite horizon and so there is no terminal period solution.

When cycles == N, it's a finite model repeated many times. If a terminal solution is given, the solver returns N * T + 1 solutions.

[Mv77]

This looks good to me and tests pass, but I have never worked with cyclical models in HARK so I am not to confident in my capacity to catch a bug here.

[sbenthall]

@mnwhite want to double check this before I merge?

[sbenthall]

@mnwhite unless you have objections to this I'll merge this soon.

[llorracc]

@sbenthall

I haven't had time to look at this, but while it's on my mind let me ask something I've been a bit confused about from the beginning of the descriptions of what you are doing.

What confuses me is that there's nothing "wrong" about the way things have been done since the beginning. It's perfectly acceptable to have some shocks be at the end of the period (that is, an EOP shock dated t is something whose parameters should properly live on solution[t]).

Problem is, most of our LaTeX and conceptual descriptions of what we are doing correspond to the shocks that matter between t and t+1 being realized at the beginning of t+1, but our code had them being realized at t.

My pre-alpha-HARK-2.0 PR does it right (in a mathematical and conceptual sense), by allowing both BOP shocks or EOP shocks, and defining an infrastructure for handling either kind (or both kinds).

So what's confusing me is that much of the discussion of all this seems to describe the shocks as being "on the wrong period" and as "moving" them. My pre-alpha-HARK-2.0 does not need any change at all in the method of construction of the shocks and I can't quite see how a solution to the discrepancy between our LaTeX and our python description requires "moving" anything. It just allows a variable 'shockTiming' which tells the code how to interpret the shocks that were already being constructed: They should be interpreted (by the code) as BOP not EOP.

I'm guessing that all the stuff you have done amounts to modifying the solution and simulation code to ASSUME that the shocks are at the BOP not the EOP. But, really, the right solution is not to hard-wire either of them as being "right" or "wrong" -- both kinds of shocks are legit -- but to write code that correctly understands whether shocks of with any given name are BOP or EOP.

If there are going to be BOP and EOP shocks, probably we should have a convention for their names that makes it easy to read the code and figure out what is going on. We could have IncShkDstn_BOP and IncShkDstn_EOP.

I think what you have done is that you've reworked the code to assume the shocks are BOP. That's probably the best way to make this fix: If shocks do not have an explicit label, the default assumption should be that they are BOP instead of EOP.

So, I guess (I hope) that my point is that it's confusing to talk about what is being done as it has been talked about, as though the code before had a "mistake." The code was producing a perfectly valid solution to a perfectly valid problem. It just that:

1. The problem being solved did not correspond exactly to the LaTeX description of it;
2. Substantively, what the LaTeX code describes is a more attractive way to do it;
3. So our code should be solving a (slightly) different model than what it was solving before

This requires, for example, the creation of a BeginningOfPeriod_vP object, and the redefinition of EndOfPrdvP to be essentially just the discount factor times BeginningOfPeriod_vP.

[llorracc]

@sbenthall, @Mv77

To clarify the relationship between the previous comment and the discussion of the "cyclical" code: There's nothing wrong with the original code, which correctly requires the existence of a "solution_next" and constructs the current solution accordingly. In this case, the "solution_next" for the last stage of the "cycle" would correspond to the solution to the first stage of the next cycle. That was true before and it's true now, so I'm a bit confused about what needed to change.

[sbenthall]

The problem I am trying to fix is that before this PR, there was no way to specify in a model's parameters what the shock values of the 0th period are.

These had to instead be hard-coded into the simulation code for the model. That reduces the generalizability of the software.

The shock value parameters for the 0th period are not used in the current solution methods. That is why they weren't included originally. But that is not a good choice if you're trying to provide a flexible modeling tool.

From my perspective, the question of whether shocks happen at the beginning or end of a period is an utter red herring, and creates more confusion than it settles. I abstain from further comment on that issue. However, I do know that every time we have discussed the changes I've made here (and corresponding changes proposed for the non-cyclical models), there has ultimately been consensus between you, myself, and @mnwhite. That's good enough for me!

[llorracc]

I'm in agreement that we needed to do something systematic about the inability to specify shocks that occurred before the agent's first actions in the first simulated period; that is accomplished by allowing beginning-of-period shocks, as in my new code.

For a variety of reasons, economic, notational, and computational, it makes a great deal of sense to allow shocks within the period either before or after decisions are made.

But I agree that whenever you and Matt and I have discussed these issues in depth, we seem to have reached agreement conceptually. So I'm pretty confident the questions in my prior message would evaporate with sufficient discussion. It's more that I wanted to record a bit of puzzlement about some of the ways you've been talking about things, like whether the last period of this cycle should properly refer to "next" as the first period of the following cycle. Since I can't see any reason we would change that, and it doesn't seem to have anything to do with the time t issue, I was worried we had somehow misunderstood each other.

[sbenthall]

I see what you're saying now.

To adopt your terminology: I believe that prior to this PR, and the corresponding change being discussed in #1042, it was the case that for all HARK models, the shocks were BOP. However, the solution code, and more specifically the way models get parameterized, which what what I'm really trying to address here, was written as if they were EOP.

That mismatch was incorrect, and is what's being fixed by these changes.

I see now that you are talking about how you think EOP shocks should be supported by HARK, even though none of the current models in HARK use EOP shocks.

The confusion is perhaps settled by the understanding that I'm writing PRs pragmatically using the current state of HARK as a reference. Maybe you should create an issue for EOP shocks in which we could discuss a practical plan for implementing those.

I believe EOP shocks will be supported in the FramedAgentType architecture I'm pitching in #865

[llorracc]

Yes, you've got it now -- and as expected, now that we understand each other, we are in agreement. Thanks.