-
-
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
Portfolio model with income deduction scheme. #832
Conversation
You are right, sorry about the docstrings. I used to keep them in the same file but then thought it would be better to have them in separate files, given the nature of There are various forthcoming models in HARK that could be described as "A
Does this make sense? |
|
||
|
||
# Class for the consumption stage solution | ||
class RiskyContribCnsSolution(MetricObject): |
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.
further in that logic... maybe the stage names should have longer, more legible names. But then appear less often, because the code is structured to use that information efficiently.
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've deleted the redundant tags from class properties.
However, some degree of redundancy might help the reader. For instance, while solving the contribution share stage, I know that v_func_next
will be of the Cns
type, because the consumption stage comes next. However, somebody who is just figuring out what is going on in the model might thank calling the object v_func_Cns_next
just so that he does not have to even think of it , or scroll up to the docstring.
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's very hard to judge the aesthetics of these things since there is great variation in use of text editors, IDEs, documentation, etc.
I believe a more compact representation is standard across scientific libraries. If you examine the API documentation of many of the packages that we depend on, I believe you will come to see a convergence on a certain style.
@Mv77 thanks, that makes sense. I hadn't seen #1012 -- maybe it was while I was away? -- but the rationale for it makes sense. It would help if its documentation noted that the absence of the solution code was deliberate. Indeed, it says "This file contains classes and functions for representing, solving, and simulating agents who must allocate their resources among consumption, saving in a risk-free asset ...", which is false, because it does not in fact contain the solution code.... But other than that I have no objections to the code shuffling! One thing that has been on the agenda for a very long time is #495 -- we would actually like it if the model definitions and solution code were more decoupled. I'd be interested if your experiences with this have shed any light for you on the best way to go about doing that. I'm going to be tinkering with this in more work on #865 soon and I hope I'll be able to get your review on that when it's ready. |
**unused_params | ||
): | ||
|
||
# Make sure the individual is liquidity constrained. Allowing a consumer to |
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.
This single function is 250 lines long.
My suspicion from working with HARK is that much of this complexity is due to the hard-coding of mathematical operations that could and should be functionalized out into reusable subroutines.
I wonder if there is any way to shorten this and the other solution methods?
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.
That's a fair criticism. I'll see what I can do.
Incidentally, this is another pro of the "splitting into stages" strategy. Imagine if these were not 3 250-line functions but a single 750-line function.
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've spun my wheels a bit on this and I don't see a reduction or partition that would result in functions that would be useful in other models. I could split the solve_*
functions in parts but they would be specific to this model and require more arguments being juggled around, complexity would increase in my opinion.
You are right that a lot of complexity and raw code length comes from operations that could be abstracted. But I'm using a host of those things that we've generalized already e.g.
- The general n-dimensional
CRRA(Marg)ValueFunc
that are now inHARK.interpolation
are a spin-off of this project. - You'll be pleased to see that expectations are computed using your
calc_expectations
function.
I understand your point. But some of these things are hard to abstract. For instance, one might think of working towards a routine that "finds an optimization problem's solution given its first order condition". But in my model:
- The FOC is necessary and sufficient in the
Cns
stage. Yet, it is used in a very idiosyncratic way (EGM). This way of using it is worth it because EGM is the most efficient way one can solve a consumption problem if it is applicable. - It is not necessary or sufficient in the
Reb
orSha
stage because of the possibility of solutions that are not interior (Sha
) or kinks in the objective function (Reb
). Still, the FOC contains information that allows you to find the solutions.
It's these idiosyncrasies that result in this "hard-coding" of solution methods. But it might be something we might have to live with. The opposite extreme alternative would be to use scipy.optimize
to create a solver that would apply to any bellman equation. But this solver would not have a chance of solving the kinds of models we are interested in. We are incorporating analytical insights of specific models to reduce numerical optimization demands.
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.
Thanks for thinking this through.
I was concerned when I mentioned it that it might be an unfair point to make; it's certainly not blocking the PR, which I intend to merge soon.
It looks like many of the lines are documentation and idiosyncrasies of the black
formatting tool.
I think it should be possible to write a generic EGM solver but that is well beyond the scope of this issue.
It was, yes.
Agreed. It was an error. Addressed in 13c35cf |
@sbenthall thank you very much for the thorough review and all the comments. I have addressed them all by either pointing to a commit that fixes them, or by writing a reply. |
Excellent. I'll merge now. Thanks, @Mv77 |
This PR contains (and will be updated with) my work on a consumption-savings-portfolio model with the following characteristics: