-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
ConsRiskySolver #1108
ConsRiskySolver #1108
Conversation
first pass at ConsRiskySolver
Codecov Report
@@ Coverage Diff @@
## master #1108 +/- ##
==========================================
- Coverage 74.07% 72.80% -1.27%
==========================================
Files 70 70
Lines 10838 11156 +318
==========================================
+ Hits 8028 8122 +94
- Misses 2810 3034 +224
Continue to review full report at Codecov.
|
update patience factor when return is risky
self.hNrmNow = ( | ||
self.PermGroFac / self.Rfree * (self.Ex_IncNext + solution_next.hNrm) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llorracc still thinking about what the appropriate discounting is for human wealth.
case 1 - divide by self.RiskyAvg
case 2 - calculate expectation of self.PermGroFac * (self.Ex_IncNext + solution_next.hNrm) / risky_shock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we decided to impose explicit liquidity constraints, so if I understand things the question is really just about the right interest factor to use to discount to end-of-$t$ the minimum possible income in
If we are going to have only one return factor, the answer is that the minimum notional
where
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this point, but my question is not related to the borrowing constraint. Human Wealth and the MPCs are used when constructing the upper limit of the consumption function as in
HARK/HARK/ConsumptionSaving/ConsIndShockModel.py
Lines 731 to 732 in 4bc5984
self.cFuncLimitIntercept = self.MPCminNow * self.hNrmNow | |
self.cFuncLimitSlope = self.MPCminNow |
adjust natural borrowing constraint to lowest transitory shock; BoroCnstArt == 0.0
create asset grids depending on whether natural borrowing constraint is zero or not
switch out old grids for new grids
impose using the class method for this
fix asset grid; add 0.0 if it's not the natural borrowing constraint
remove inserting zeros
IndepDstnBool to deal with correlated distributions
This PR now provides a working solver for the To do:
|
remove `black` import
reformat code
create functions for repeated code
create `ConsBasicPortfolioSolver`
create copy of init_portfolio to avoid circular loop
run example to explore results
add share limit and share function
move example to ConsPortfolio folder
update example notebook and sync with jupytext
scipy is too slow, use share grid instead Note: why is share grid from 0 to 1? it could be from share limit to 1 instead
I also include an example notebook to compare solution with |
the share grid only needs to go from shareLimit to 1, not below
this part of the code does not seem to work yet, so this allows for linear extrapolation after last point
adjust borrowing constraint
@@ -173,6 +172,8 @@ def __init__(self, verbose=False, quiet=False, **kwds): | |||
params.update(kwds) | |||
kwds = params | |||
|
|||
self.PortfolioBool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the documentation for this variable? It could go in the docstring.
Why is this variable named in CamelCase rather than snake_case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to name it in a similar pattern to CubicBool
and vFuncBool
RiskyVar = agent.RiskyStd ** 2 | ||
RiskPrem = agent.RiskyAvg - agent.Rfree | ||
RiskyVar = agent.RiskyStd**2 | ||
RiskPrem = agent.RiskyAvg - agent.Rfree[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the lack of parallelism between RiskyAvg (a scalar) and RFree (a time-varying something) at all a cause for concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think RiskyAvg depends on an input of the portfolio share, and thus is not something whose dynamics are predetermined like those of Rfree? But if that's right, shouldn't it be Rport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RiskyAvg
is a model parameter, but not a solver parameter, so it is handled differently than Rfree
. Depending on it's nature, the agent constructs a Distribution
that will be set to time varying or time invariant. Rfree
however is now expected to be time invariant, so even when it's the same throughout the lifecycle, as is here, the agent class holds the value as a list.
What @llorracc is talking about is the method get_Rfree()
which does make a weighted combination of the risky return (distributed ~ lognormal) and the Rfree
to return Rport
.
In this way, Rfree
behaves like PermGroFac
or LivPrb
which are always lists and are solver parameters.
Perhaps what you are implying is that RiskyAvg
should also be handled as time-varying, as are the income distributions.
@@ -470,26 +473,26 @@ def add_mNrmStE(self, solution): | |||
# All combinations of c and m that yield E[PermGroFac PermShkVal mNext] = mNow | |||
# https://econ-ark.github.io/BufferStockTheory/#The-Individual-Steady-State | |||
|
|||
PF_RNrm = self.Rfree/self.PermGroFac | |||
PF_RNrm = self.Rfree / self.PermGroFac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general ... the reformatting across the library makes it harder to figure out the changes made in this specific PR.
You might consider making a separate PR with just these formatting changes, which could be merged easily.
That would help reviewers focus on the substantive changes in code.
RiskyAssetConsumerType = IndShockRiskyAssetConsumerType | ||
|
||
|
||
class FixedPortfolioShareRiskyAssetConsumerType(IndShockRiskyAssetConsumerType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It strikes me as odd that:
- IndShockRiskyAssetConsumerType has two 'modes' : strategic share variation, or with a fixed rate at 1
- FixedPortfolioShareRiskyAssetConsumerType is subclass of the above class, with a variable fixed rate.
If I'm not mistaken, the fixed-rate-of-1 agent is a special case of the fixed-rate agent, and so logically, the former should be a subclass, if not a specific parameterization, of the latter
Also...these class names are getting to be a little ridiculous. There has got to be a way to shorten them. This may entail a larger conversation about model class naming conventions.
|
||
|
||
@dataclass | ||
class ConsIndShkRiskyAssetSolver(ConsIndShockSolver): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a deep insight into this, but superficially, I'm surprised that the three solver classes are each so long.
I would have expected more of an opportunity for code reuse between these solvers, given their similarity.
I wonder whether there's any functional redundancies in these classes that could be factored out.
First pass at
ConsRiskySolver
as requested in #1107Still a work in progress as I have to figure out if all the additional functionality of
ConsIndShockModel
is appropriate for a model with risky asset returns. For example:This PR also showcases the use of a dataclass as a solver class. I like this a lot more than the enumeration of parameters in
__init__
and it doesn't lose theget_arg_names
functionality.Please ensure your pull request adheres to the following guidelines: