[WIP] Add stable points for PortfolioConsumerType#1259
[WIP] Add stable points for PortfolioConsumerType#1259alanlujan91 wants to merge 4 commits intoecon-ark:mainfrom
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 72.55% 72.59% +0.04%
==========================================
Files 78 78
Lines 13009 13028 +19
==========================================
+ Hits 9439 9458 +19
Misses 3570 3570
☔ View full report in Codecov by Sentry. |
|
Alan, Seb has a PR to the LucasAssetPrice that has a formula that, if it is correct, should be the right impatience condition for the portfolio choice model (when the risky return from the standpoint of the portfolio choice consumer is the portfolio-weighted return). I don’t have time to review Seb’s PR right now — I did the derivations for the approximate formula, which were what was needed for the pedagogical purpose of the handout, and Seb has tried to turn that into the exact formula in levels (that’s what his PR does). If you could review that, then incorporate that formula into the PortfolioConsumerType model (with a comment linking to the LAP lecture notes as the justification), that would be great. cc’ing Akshay because I’ve mentioned to him that this would be a useful small extension to the derivations in the BufferStockTheory paper that he is reviewing for me. Presumably the condition in my LAP model is identical to impatience formulas in Stachurski, Ma, and Toda (if I am right that their model is one with only rate-of-return risk). PS. I've added all three of you as collaborators, so that Alan can merge Seb's PR after review. |
alanlujan91
changed the title
There was a problem hiding this comment.
Pull request overview
This work-in-progress PR attempts to add stable point calculations (mNrmStE and mNrmTrg) to the PortfolioConsumerType by inheriting functionality from ConsIndShockSolver. The PR author acknowledges this approach might not be appropriate for the portfolio model but provides a starting point.
Changes:
- Changed ConsPortfolioSolver to inherit from ConsIndShockSolver instead of MetricObject
- Added attributes (mNrmMin, hNrm, MPCmin, MPCmax) to terminal solutions to support parent class methods
- Integrated add_MPC_and_human_wealth and add_stable_points methods into the solve process
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| HARK/ConsumptionSaving/ConsRiskyAssetModel.py | Removed inline comment from import statement (minor cleanup) |
| HARK/ConsumptionSaving/ConsPortfolioModel.py | Changed solver inheritance structure, added terminal solution attributes, integrated stable points calculation into solve methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| super().set_and_update_values( | ||
| self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac | ||
| ) |
There was a problem hiding this comment.
The method signature declares parameters (solution_next, IncShkDstn, LivPrb, DiscFac) but then ignores them and uses self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac instead when calling super(). This is confusing and inconsistent. Either remove the unused parameters from the signature, or use the parameters instead of self.* attributes.
| super().set_and_update_values( | |
| self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac | |
| ) | |
| super().set_and_update_values(solution_next, IncShkDstn, LivPrb, DiscFac) |
| super().set_and_update_values( | ||
| self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac | ||
| ) |
There was a problem hiding this comment.
The method signature declares parameters (solution_next, IncShkDstn, LivPrb, DiscFac) but then ignores them and uses self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac instead when calling super(). This is confusing and inconsistent. Either remove the unused parameters from the signature, or use the parameters instead of self.* attributes.
| super().set_and_update_values( | |
| self.solution_next, self.IncShkDstn, self.LivPrb, self.DiscFac | |
| ) | |
| super().set_and_update_values(solution_next, IncShkDstn, LivPrb, DiscFac) |
| self.solution = self.add_MPC_and_human_wealth(self.solution) | ||
| self.solution.mNrmMin = self.mNrmMinNow | ||
| self.solution = self.add_stable_points(self.solution) |
There was a problem hiding this comment.
No tests have been added to verify the stable points functionality (mNrmStE and mNrmTrg) for the portfolio model. Given that this is a new feature and there are existing tests for stable points in test_IndShockConsumerType.py and test_PerfForesightConsumerType.py, tests should be added to verify that the stable points are correctly calculated for portfolio consumers.
| self.solution_next.vFunc = self.solution_next.vFuncAdj # this is a hack | ||
| self.solution_next.vPfunc = self.solution_next.vPfuncAdj # this is a hack |
There was a problem hiding this comment.
The comment "this is a hack" is not sufficiently descriptive. Consider expanding it to explain why this assignment is necessary, something like: "Map portfolio-specific vFuncAdj and vPfuncAdj to the generic vFunc and vPfunc names expected by the parent class's set_and_update_values method."
| self.solution_next.vFunc = self.solution_next.vFuncAdj # this is a hack | |
| self.solution_next.vPfunc = self.solution_next.vPfuncAdj # this is a hack | |
| self.solution_next.vFunc = self.solution_next.vFuncAdj # Map portfolio-adjusted value function to generic vFunc name expected by the parent class's set_and_update_values method. | |
| self.solution_next.vPfunc = self.solution_next.vPfuncAdj # Map portfolio-adjusted marginal value function to generic vPfunc name expected by the parent class's set_and_update_values method. |
| self.solution = self.add_MPC_and_human_wealth(self.solution) | ||
| self.solution.mNrmMin = self.mNrmMinNow | ||
| self.solution = self.add_stable_points(self.solution) |
There was a problem hiding this comment.
The add_stable_points method inherited from ConsIndShockSolver expects the solution object to have a cFunc attribute. However, PortfolioSolution uses cFuncAdj and cFuncFxd instead of cFunc. When add_stable_points calls add_mNrmStE or add_mNrmTrg, these methods will attempt to evaluate solution.cFunc(m), which will fail with an AttributeError. To fix this, either add a cFunc attribute to PortfolioSolution that maps to the appropriate function (likely cFuncAdj), or override add_stable_points in ConsPortfolioSolver to handle the portfolio-specific solution structure.
| self.solution = self.add_MPC_and_human_wealth(self.solution) | ||
| self.solution.mNrmMin = self.mNrmMinNow | ||
| self.solution = self.add_stable_points(self.solution) |
There was a problem hiding this comment.
The add_stable_points method inherited from ConsIndShockSolver expects the solution object to have a cFunc attribute. However, PortfolioSolution uses cFuncAdj and cFuncFxd instead of cFunc. When add_stable_points calls add_mNrmStE or add_mNrmTrg, these methods will attempt to evaluate solution.cFunc(m), which will fail with an AttributeError. To fix this, either add a cFunc attribute to PortfolioSolution that maps to the appropriate function (likely cFuncAdj), or override add_stable_points in ConsPortfolioSolver to handle the portfolio-specific solution structure.
| @@ -1220,6 +1250,10 @@ def solve(self): | |||
|
|
|||
| self.make_porfolio_solution() | |||
|
|
|||
There was a problem hiding this comment.
The add_stable_points method inherited from ConsIndShockSolver expects the solution object to have a cFunc attribute. However, PortfolioSolution uses cFuncAdj and cFuncFxd instead of cFunc. When add_stable_points calls add_mNrmStE or add_mNrmTrg, these methods will attempt to evaluate solution.cFunc(m), which will fail with an AttributeError. To fix this, either add a cFunc attribute to PortfolioSolution that maps to the appropriate function (likely cFuncAdj), or override add_stable_points in ConsPortfolioSolver to handle the portfolio-specific solution structure.
| # Ensure the solution object has a cFunc attribute for methods inherited | |
| # from ConsIndShockSolver that expect solution.cFunc(m). | |
| if not hasattr(self.solution, "cFunc"): | |
| if hasattr(self.solution, "cFuncAdj"): | |
| self.solution.cFunc = self.solution.cFuncAdj | |
| elif hasattr(self.solution, "cFuncFxd"): | |
| self.solution.cFunc = self.solution.cFuncFxd |
|
|
||
| self.solution = self.add_MPC_and_human_wealth(self.solution) | ||
| self.solution.mNrmMin = self.mNrmMinNow | ||
| self.solution = self.add_stable_points(self.solution) |
There was a problem hiding this comment.
The calculation of stable points using the ConsIndShockSolver methodology may not be conceptually appropriate for the portfolio model. The stable points (mNrmStE and mNrmTrg) in ConsIndShockModel represent target wealth levels assuming a single consumption function. However, in the portfolio model, consumption depends on both market resources and the risky portfolio share, with different functions for when the agent can adjust (cFuncAdj) versus when they cannot (cFuncFxd). The stable point calculations should likely account for the portfolio allocation decision and adjustment probability, not just use the adjustable consumption function. Consider developing a portfolio-specific stable points calculation that properly incorporates the two-dimensional nature of the problem.
| self.solution = self.add_stable_points(self.solution) | |
| # Stable points from ConsIndShockSolver are not conceptually appropriate | |
| # for the portfolio model (which is two-dimensional and has cFuncAdj/cFuncFxd). | |
| # Explicitly mark them as unavailable instead of using the 1D logic. | |
| self.solution.mNrmStE = None | |
| self.solution.mNrmTrg = None |
|
|
||
|
|
||
| class ConsPortfolioSolver(MetricObject): | ||
| class ConsPortfolioSolver(ConsIndShockSolver): |
There was a problem hiding this comment.
This class does not call ConsIndShockSetup.init during initialization. (ConsPortfolioSolver.init may be missing a call to a base class init)
This is inheriting the way to calculate stable points from
ConsIndShock, which might not be appropriate forPortfolioConsumerTypebut provides a starting point.