econ-ark / HARK

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

Update Wealth portfolio solver #1404

Closed alanlujan91 closed 5 days ago

alanlujan91 commented 1 month ago

Please ensure your pull request adheres to the following guidelines:

mnwhite commented 1 month ago

It looks like this PR makes changes to ConsIndShock as well, moving some functions out of the solver. Looking through that, there's at least one actual code change, to MPCmaxNow vs MPCmaxEff. These are not (necessarily) the same value. The solver constructs the unconstrained consumption function and combines it with the constrained consumption function via LowerEnvelope. MPCmaxNow is the MPC at the lower bound of the unconstrained function, whereas MPCmaxEff is the effective maximum MPC for the actual consumption function. MPCmaxNow shouldn't be overwritten by the "should this just be 1?" line, because its value is still used later on.

llorracc commented 1 month ago

Am in agreement with Matt. But maybe to prevent confusion for future users, the name of MPCmaxNow should be changed to something like MPCmaxIfNoLiqConstr and perhaps a comment could be added to the code to explain why it is computed and what it means.

alanlujan91 commented 1 month ago

This PR builds on #1395, so it's more appropriate to discuss MPC constructions there and leave this for later

mnwhite commented 3 weeks ago

This is on my immediate list.

alanlujan91 commented 3 weeks ago

@mnwhite can you take a look now?

mnwhite commented 2 weeks ago

I haven't gone through every single line of math in the auxiliary functions, but I have three pieces of feedback on the solution method:

1) When I did the math/code for the "chi from omega" trick, I didn't know there was such a thing as the WealthShift parameter. The existence of that parameter changes things significantly for two reasons. First, I would want to go back and redo the math with that shifter in place to make sure everything goes through without complication; I put about 80% confidence on it, but that's not very high. Second, it means the functional representation isn't great: we'll never actually approach one of the limits of the function, and there might be a better representation due to that. I strongly recommend writing out the math by hand from scratch with WealthShift to be super duper extra sure that all of the aNrm in the math without it can just be replaced with aNrm+WealthShift.

2) I need to double check the envelope condition math. When I first looked at the code, I thought "this isn't right", but after writing out my reasoning, I'm revising that to "this isn't how I would have done it", and I need to do the math myself. I now think what you've done is probably correct.

3) We/you should do some robustness checking on the logic of whether/when to add an extra gridpoint for aNrm=0. I think the proper logic is more complicated than you have, and should involve a check of whether WealthShift > 0. My concern is that there are situations where two gridpoints are generated at exactly mNrm = 0.

alanlujan91 commented 2 weeks ago

@mnwhite

  1. I believe everything else is the same, except chi is the ratio of c/(a + a_bar), the presence of a_bar doesn't change any derivatives since it's a constant
  2. Happy to do this a different way, this was just the best way I could come up with that would be consistent with WealthShare == 0 aka the simple Portfolio problem. I don't like that I had to do a conditional for WealthShare == 0 when solving optimal consumption, I would have liked a solution that worked for either.
  3. I am essentially checking for (a + WealthShift) > 0.