PortfolioFrameModel and FrameAgentType

[sbenthall]

This PR addresses issue #862, with some movement towards #660 and #798

The goal is to provide a general architecture for models that use the "frame" method of defining a model.

The main changes are:

• FrameAgentType, a subclass of AgentType that changes the simBirth and simOnePeriod methods to make use of model configuration objects birth_values and frames
• A ConsPortfolioFrameModel which is a conversion of the existing ConsPortfolioModel to use FrameAgentType.

The current implementation passes all automated tests, which have been converted from the original ConsPortfolioModel tests to use the new class. What that implies is that strictly speaking, this is just a refactoring of the old class code!

I have a few open design questions:

• How should the FrameAgentType class deal in general with the setting of aggregate variables such as PLvlAggNow?
• In order to make it as consistent as possible with the original code, I've defined the transition methods so that they mutate the underlying model variable storage. I personally think it's cleaner/more mathematical/more aligned with Dolo if the transition methods return the variable values, and the FrameAgentType handles the storage of the values in memory. I wanted to run this option by you.

• 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.

[sbenthall]

This PR now had CHANGELOG edits, updated docs, and full tests.
I think it's ready to merge.

[Mv77]

I like what you have done here, fully specifying states, shocks, controls and aggregates.

When creating new models, a frustration of mine has been the inconsistency with which that specification is done across different HARK agent types. Its sometimes done, sometimes not, sometimes inherited so one has to go hunting for the original class' attributes, sometimes fully specified.

It is further complicated by two different objects shock_vars and shock_vars_ (see e.g.) with no indication of what each of them does. I think I have seen this for states too.

[sbenthall]

What I would like to do ultimately is move all of this information into the frame definitions.
Then have the constructor code build these dictionaries based on those frames.
Ultimately, I want all model information in the frames dictionary, and nothing baked into a model class.
This is sort of an intermediary implementation.

[Mv77]

was the _now not out of fashion now?

[sbenthall]

These are doing some logical work as per the current HARK simulation code.
There, there's both state_now and state_prev, because transition functions can depend on the previous period's state.

I can see how we could do an implementation where the state memory array was duplicated each period, and updated frame-by-frame.

That's an implementation detail, as far as I'm concerned, not a matter of model logic. I'm not opposed to making the implementation consistent for shocks, controls, and states, as another PR. But there may be benefits to keeping state_prev.

[Mv77]

the the

[sbenthall]

I want to note one thing in particular about the tests here.

I found that with the new architecture I was getting a slightly different sampling order for shocks. This is of course fine for the library, but it breaks the extremely brittle tests we've had which check for a specific numerical value.

In many places in this test suite, I've changed to this more resilient and accurate style of test. I'm now testing the relationships between model state/shock/etc. values, rather than particular numerical values.

That will mean that if the code is working properly, some change to the distribution sampling order won't break all the tests.

We might try to do this elsewhere in the library before doing a big random number generation shifting change, such as #1026

[llorracc]

@sbenthall

That's great, thanks, you are right that our existing tests are much too finicky and I'm glad to learn that you are robustifying them. To tell the truth, even 6 decimal places (which is what we have dialed down to from a default of 13) is far more than makes much sense for most of our tests. 3 or 4 digits is probably more than enough.

[Mv77]

Something that would make me confident about the PR would be to have a test where a ConsPortfolio and ConsPortfolioFrame agent types were created and solved with the default parametrization and their solutions and simulations were compared.

[sbenthall]

That's an interesting idea.
I don't think a comparison between the solutions will be interesting because it's literally just calling the same solution code.
Though I could do a few tests to verify that it's working as expected.

Comparing the simulations would need to take into account a couple things:

• differences in sampling. i suppose these could be done by averaging many cases.
• the fact that in this model the simulation is computing a stochastic transitory shock for the 0th period of life, as opposed to the current default code that for some reason makes this shock 1.0.

Still, a side-by-side plot would be easy to do.

[Mv77]

I like the direction of the PR and how it makes the simulation code clearer.

I suppose the solution side is the next hurdle to be cleared.

[llorracc]

@sbenthall,

Reviewing this seems like something that would be best accomplished, at least in the initial stage, via Zoom. In particular, if you have a notebook as well as the code so you could do something like what I did last time with the pre-alpha HARK 2.0 post, where the notebook was demonstrating the substantive outcome but I also had the code up and Spyder running so we could look at the internals, that would be great.

@Mv77 is in Bonn now (I think) communing with Fedor and the Great Spirits of dynamic structural estimation (or some of them), and it would probably be good to have him on the call. So maybe a week from Thursday would be a good target?

[sbenthall]

Thanks for the review @Mv77
I'll make some edits based on your comments, and try to move forward on a couple things I've mentioned (e.g. aggregation defined in frames).

That would make it polished for August 19th, which @llorracc you're suggesting as a review date.

I don't know if there's much in terms of new substantive results to this; it's mainly a refactoring job. But I can think about what would be useful for demo purposes. I am mainly hoping to have this PR go smoothly so that I can start working on generic solution code.

[Mv77]

@llorracc I'm still in the US! I leave on the 14th and come back on the 23rd. I can chat this week if you would like.

An issue is that I have my pre-trip covid test on Thursday morning, so I can't make the usual HARK call.

[sbenthall]

Ok, I'm glad I compared the ConsPortfolioModel and the new PortfolioFrameModel simulation results directly.
I see now I'm getting some buggy behavior when sampling the shocks.
I hope to add repairs to the PR soon.

[sbenthall]

I have repaired this PR.
Now Risky is an aggregate shock and Rport is computed as a separate frame.
(This shows, by the way, how this new architecture frees us from the misnamed get_Rfree method that was needed for backwards compatibility.)

The notebook in examples/FrameAgentType shows a comparison between a simulation with the new class and the old class.

Requesting review (again... thanks!) from @Mv77

[Mv77]

No worries, I am a committed committer now. I'll slowly but surely get through it. :)

[Mv77]

This looks good to me.

My only question just to make sure is: the simulations in the example do not exactly match just because of the RNG, right? The frame type is sampling different shocks at different points of execution with potentially different seeds from those of the regular portfolio type?

[sbenthall]

Thanks @Mv77

Yes, I currently believe that the change in random seed accounts and some of the sampling steps account for the discrepancies.

For example, the FramedAgent using IndexDistributions does not sample values for newborns twice (see #1021).

However, it is possible that there are remaining bugs in my code that result in other differences. These are naturally difficult to detect. If we can't find them, I hope we wouldn't let the perfect be the enemy of the good.

The new example notebook is designed to show and compare the two simulations.

[Mv77]

Yes, for sure. Just wanted to make sure I understood. I approve!

…

On Wed, Sep 1, 2021, 8:29 PM Sebastian Benthall ***@***.***> wrote: Thanks @Mv77 <https://github.com/Mv77> Yes, I currently believe that the change in random seed accounts and some of the sampling steps account for the discrepancies. For example, the FramedAgent using IndexDistributions does not sample values for newborns twice (see #1021 <#1021>). However, it is possible that there are remaining bugs in my code that result in other differences. These are naturally difficult to detect. If we can't find them, I hope we wouldn't let the perfect be the enemy of the good. The new example notebook is designed to show and compare the two simulations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#865 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGTULS3EFIDRXIPULOQ5GYLT73AP5ANCNFSM4TN6PILQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[llorracc]

small suggestion: effective way to detect bugs in this kind of context is often to dial down the variance of the shocks to zero, or very close to zero. Bugs (if they exist) will often leap out at you when you do that. Then, dial up one but not the other of the shocks. etc.