econ-ark / HARK

Heterogenous Agents Resources & toolKit
Apache License 2.0
315 stars 195 forks source link

Add constructor architecture #1410

Open mnwhite opened 1 month ago

mnwhite commented 1 month ago

Beginning work on the construct method of AgentType. As of this moment, a basic prototype works for income distributions. Still a lot more to develop and test.

mnwhite commented 1 month ago

As noted in the issue where I proposed this, there are two significant design points to work out:

1) How to handle situations where it's natural to construct multiple objects in the same function. The most obvious case is the first example I implemented: IncShkDstn, PermShkDstn, and TranShkDstn. I put in an ugly workaround, but something else should be done before this is merged.

2) Whether and how we want constructors to indicate whether they've made a time-varying or time-invariant object. I'm beginning to come around to the idea that these concepts (to the extent they're in HARK now and will be until several more overhaul steps are completed) should not be intended to be changed at the instance level. Rather, they're a fixed fact of the AgentType. There's a very specific reason they were defined at the instance level, but that reason no longer exists.

mnwhite commented 3 weeks ago

Can someone tell me how to run the pre-commit routine locally? I can run black, but I don't know how to make lint work.

sbenthall commented 3 weeks ago

$ pre-commit run --all-files

sbenthall commented 3 weeks ago

more info in the developer guide here: https://docs.econ-ark.org/guides/contributing.html#pre-commit-hooks

alanlujan91 commented 3 weeks ago
pip install pre-commit # install 
pre-commit install # start-up
git add * # stage your changes
pre-commit # run the hooks
alanlujan91 commented 3 weeks ago

$ pre-commit run --all-files

you probably don't want to run all files, only your changes

mnwhite commented 3 weeks ago

A bunch of tests needed syntax changes, but everything passes now. The other things that I'm moving to constructors will be much easier from here on out, and should be completed on Monday.

For this PR, I'm going to set the goal at getting everything in constructor functions that should be (plus some alternates that can be swapped in). After this is merged, in a separate branch/PR, I'm going to tackle undoing/simplifying the custom init methods for each AgentType subclass, and get them all just using AgentType.init ... That also means that the default dictionary should be a class attribute.

alanlujan91 commented 3 weeks ago

We should be doing super().init everywhere

mnwhite commented 3 weeks ago

Even where super().__init__ is used, there can be problems when update() is called by a parent class as part of init before the child class is "ready" for it, leading to unexpected results. In the not-too-distant future, I hope all our AgentType subclasses inherit directly from AgentType, and don't have any special init stuff.

mnwhite commented 1 week ago

If tests pass on this commit, the next commit will change exactly one line of code, and revise tests to fit. As is, the "construct pLvlGrid by simulation" method draws on AgentCount, which is fine as long as AgentCount is actually large. The number of eventual simulated agents shouldn't be used in the solution of the problem, even extremely indirectly. So I'm just going to have it use a large number of agents no matter what. This is also why the hardcoded seeds don't matter in this function/method.

mnwhite commented 1 week ago

The remaining tasks on this PR are pretty small, almost optional:

I'm going to try to finish this by Friday, as it has fallen behind schedule.

llorracc commented 1 week ago

Also: Is this ready for use? Ryan Edwards is working on making a new Aiyagari REMARK using HARK and I want him to use the latest and greatest tools ...

mnwhite commented 1 week ago

This also allows for transitory shocks, I assume?

Someday I want a tool that can handle the case that I think captures almost all of what can be said about annual income processes: It's hard to do better than a process with a fully permanent shock and a transitory shock with an MA(1) term (as time aggregation would imply). I take it that your setup here allows for purely transitory terms but probably not for MA(1) transitory terms?

This PR is just a restructuring. I've made very small changes to the constructor methods when converting them to functions, but they're almost trivial adjustments. This isn't about economics or new capabilities at all. So all the same income processes are in there that we had before.

It might be possible to do an MA(1) transitory process using MarkovConsumerType, but I'd have to work out whether there's a better way than having N^2 discrete states, where N is the number of transitory shock values.

mnwhite commented 1 week ago

Meant to say before posting: If it's not, we can add something later that can handle this income process. Let me think about it.

mnwhite commented 1 week ago

The simplest way to do it in HARK (read: the way that requires the least new code) is to make a subclass of MarkovConsumerType with an adjusted simulation procedure. When solving the model, the Markov state can be "today's transitory shock", which simply gives the forward-looking agent a little bit of information about next period's transitory income (weighted average of today's shock and tomorrow's shock).

When simulating the model, you just need a mechanism that enforces that (the index of) today's transitory shock is the same as (the index of) today's discrete state.

mnwhite commented 1 week ago

Still behind schedule, as there were more terminal solvers than I remembered. Still to do the following small tasks:

mnwhite commented 6 days ago

Once the current tests finish, I'll be pushing a commit that only has documentation / changelog updates. I've added a small number of "alternate" constructors that can be substituted in, but there's an arbitrarily large number of other ones that could be written as well.

Following yesterday's discussion with Seb and Alan, I added an additional features to construct: the option to force it to continue running even if it encounters exceptions. In the default (force=False), any exception other than simply not finding an argument will raise an exception as normal; reaching the end of the method without having completed its work is an exception. Under force=True, it records each exception to _constructor_errors and then ignores it, terminating only when it runs out of work to do, as usual. This option allows the user to (e.g.) run construct(force=True) and then simply examine ThisType._constructor_errors (and do ThisType.describe_constructors()) to get a quick read of what happened when they tried to build all of their constructed inputs.

This PR is ready for others to look at. No external behavior has changed. The only changes in this PR are how complex objects get created.

sbenthall commented 6 days ago

This all looks very good to me.

The one thing which I think could be improved is the confusing way in which constructed objects are stored, both (!) as an entry in parameters and as a new attribute on the model object. Neither of these seem quite right to me.

What about a self.constructs dictionary?

sbenthall commented 5 days ago

Ok, I understand that removing object attribute storage entirely is a goal, but something to be implemented in a different PR.

I'll leave further notes on #889

sbenthall commented 5 hours ago

There is a conflicting file with CHANGELOG

Otherwise, I believe this is ready to merge?

mnwhite commented 1 hour ago

Fixed CHANGELOG