Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Wealth portfolio solver #1404

Merged
merged 18 commits into from
May 16, 2024

Conversation

alanlujan91
Copy link
Member

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.

@mnwhite
Copy link
Contributor

mnwhite commented Apr 10, 2024

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
Copy link
Collaborator

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
Copy link
Member Author

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

Comment on lines 333 to 355
cNrm_now = np.empty_like(aNrmGrid)

for a_idx, a_nrm in enumerate(aNrmGrid):
cNrm_guess = euler(
aNrmGrid,
a_nrm,
CRRA,
WealthShare,
WealthShift,
end_dvda_now[a_idx],
)

self.solution.shareEndFunc = self.shareEndFunc
lower_bound = aNrmGrid[cNrm_guess > 0][-1]
upper_bound = aNrmGrid[cNrm_guess < 0][0]

def solve(self):
# Make arrays of end-of-period assets and end-of-period marginal values
self.prepare_to_calc_EndOfPrd()
if self.method in ["root", "fxp"]:
self.calc_EndOfPrdvP()
elif self.method == "max":
self.calc_EndOfPrdv()
result = root_scalar(
euler,
method="toms748",
bracket=[lower_bound, upper_bound],
args=(a_nrm, CRRA, WealthShare, WealthShift, end_dvda_now[a_idx]),
)

# Construct a basic solution for this period
self.prepare_to_optimize_share()
self.optimize_risky_share()
cNrm_now[a_idx] = result.root
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnwhite could you give me your thoughts on this re-worked solver?

This problem does not have an invertible euler equation, so I instead try to do root-finding

everything else should be the same as the simple portfolio problem, and I have confirmed that when WealthShare=0.0, it is indeed the same result

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnwhite
Copy link
Contributor

mnwhite commented Apr 29, 2024

This is on my immediate list.

@alanlujan91
Copy link
Member Author

@mnwhite can you take a look now?

alanlujan91 and others added 6 commits May 2, 2024 15:40
@mnwhite
Copy link
Contributor

mnwhite commented May 7, 2024

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
Copy link
Member Author

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

@alanlujan91 alanlujan91 merged commit 77bcf90 into econ-ark:master May 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants