Parameters #1240
Parameters #1240
Conversation
|
@sbenthall need help with one failing test
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1240 +/- ##
==========================================
+ Coverage 73.31% 73.33% +0.01%
==========================================
Files 76 76
Lines 12551 12634 +83
==========================================
+ Hits 9202 9265 +63
- Misses 3349 3369 +20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
HARK/core.py
Outdated
| ------- | ||
| Parameters or value | ||
| Parameters object with parameters that apply to age `age_or_key` or value of | ||
| parameter with name `age_or_key`. |
There was a problem hiding this comment.
Maybe I'm missing something, but it feels a little odd for the return type of this in the case where the argument is an age to be a Parameters object.
The Parameters object seem to be about an Agent's entire parameterization, including the age of the agent...but what does it mean to be a Parameters object for one age slice of an agent?
Consider having the return type in the age case be a dictionary or named tuple.
There was a problem hiding this comment.
Yeah I just thought the slice of a Parameter should also be a Parameter.
We can think of Parameter objects as inputs to agents, but also as inputs to Solvers, or Simulators, or Estimators. Sometimes these objects will only take one age at a time, but I thought they should still receive a collection of parameters wrapped in a Parameters object. Then if we actually need a primitive like a dictionary or named tuple, we could easily get that from the Parameters slice.
There was a problem hiding this comment.
I am wondering to what extent this object is a novel data structure, and to what extent it has specific semantics based on its uses in modeling.
In many ways, the fundamental data structure is an array. We might accomplish a lot of the same thing with a
Of course, the difference in this case is that some parameter rows are being defined, up front, as scalars that do not vary. This is largely for convenience -- we don't want to have to manually write in long lists of constant value.
If Parameters is a data structure, then I suppose it makes sense for a slice of Parameters to be another Parameters object.
But if part of the semantics of Parameters is that it has to do with agent ages, then I wonder if the abstraction breaks down.
For example, consider:
In [1]: from HARK.core import Parameters
In [2]: from HARK.ConsumptionSaving.ConsIndShockModel import init_lifecycle
In [3]: params = Parameters(**init_lifecycle)
In [4]: params._term_age
Out[4]: 65
In [5]: param_slice = params[10]
What is param_slice._term_age?
The docs say that _term_age is "The terminal age of the agents in the model." But wouldn't param_slice be a Parameters object with no age varying parameters? Does _term_age maintain the original reference, or does it become 1?
IIRC, when you take a slice of a Python array, you wind up with a view into that array, rather than a copy of that array. This has consequences: when you mutate the slice, you mutate the original array. As in:
>>> a = np.zeros((3,3))
>>> b = a[1:2,1:2]
>>> b[0,0] = 3
>>> a
array([[0., 0., 0.],
[0., 3., 0.],
[0., 0., 0.]])
I don't think that with your current implementation, mutating slices would mutate the underlying Parameters object. That may or may not be desirable. But I hope these choices can be made in a considered way.
There was a problem hiding this comment.
The _term_age of a slice would be 1, as it re-interprets all the parameters given, which are now only for a particular time period.
The intended use of this data structure is to both handle the age dimension of models and to solve the "white board" problem.
| key : str | ||
| name of parameter | ||
| value : Any | ||
| value of parameter |
There was a problem hiding this comment.
I'm trying to get an intuition for how this feels with time-varying item setting.
I suppose if you do:
p = Parameters()
p['PermShkStd'] = [1,2,3,4]
this should set the PermShkStd parameter to the time-vary list given, and it should check to see if it's consistent with the max age of the agent, and so on?
Does this do that?
There was a problem hiding this comment.
Correct, this is how it looks in action
In [1]: from HARK.core import Parameters
In [2]: from HARK.ConsumptionSaving.ConsIndShockModel import init_lifecycle
In [3]: params = Parameters(**init_lifecycle)
In [4]: params._term_age
Out[4]: 65
In [5]: params["new_var"] = [1]*65
In [6]: params._age_var
Out[7]: ['LivPrb', 'PermGroFac', 'PermShkStd', 'TranShkStd', 'new_var']
In [8]:
| return {key: getattr(self, key) for key in self.keys()} | ||
|
|
||
| def to_namedtuple(self): | ||
| return namedtuple("Parameters", self.keys())(**self.to_dict()) |
There was a problem hiding this comment.
What about a from_namedtuple? See above about time slicing a Parameters object.
There was a problem hiding this comment.
I don't know that we need a special case for from_namedtuple. With namedtuple you can do namedtuple._asdict(), so to create a Parameters object from a namedtuple you'd just have to do Parameters(**namedtuple._asdict())
|
Thanks for working on this. See inline comments. Also, once the code is finalized some unit tests of the Parameters object would be good form. |
I will add specific tests for Parameters, but one reassuring thing is that Parameters is already being used throughout the codebase in this PR and nothing breaks with a few tweaks. |
|
This looks like a big improvement. One question: In my pseudocode for the redesign, I've decided that there needs to be a setup phase that defines things that apply to the model as a whole, like |
|
This is awesome and I look forward to it being integrated. A good check for it would be to go ahead and replace the machinery that yields parameters to |
Sounds pretty good! Would there be a mechanism to update params in ways that imply they switch from time-varying to time-invariant and vice-versa? |
Yes, every time you set a new parameter ( |
|
I'm +1 on using the explicit type of the parameter value to indicate whether or not it is time-varying. Here's an example of how I've tried implementing time-varying, non-time varying, and aggregate shocks: Here's a generic I think ideally, all the solution and simulation codes are agnostic about whether variables are time-varying or not. It just works. |
|
Related: here is how I've implemented generic Monte Carlo simulation so that it uses draws (that might be age-varying, aggegate, or not-age-varying) as part of a general simulation step. What I don't have implemented yet is selection of age-appropriate parameters for the entire population of agents (like we already can have age-appropriate draws from an Index or TimeVarying distribution). If that isn't already implemented on the Parameters class in this PR, I can add that later. |
|
Hey, My jacobian explorations have arrived at the point I need to start messing with time-varying parameters. I wanted to ask if this PR will be merged in in the near future (or is it in the back burner?) to decide how to proceed. |
|
@Mv77 The |
|
Argh yeah that sounds like an annoying job. I imagine remarks and demarks will break too once you do that. |
|
This all gets back to my fundamental point that we have not really described what an AgentType should be. In my view, it make sense to have descriptions of the solution machinery for the "solveOneStage" component of particular problems. Say, PerfForesightCRRA. And it makes sense to be able to string these stages together to define what happens in a period; say, the last stage in the problem is to solve your consumption choice, the second-to-last-stage is to define your labor supply choice, and the first stage is to define your portfolio choice. Then we might have a PortLaborConsPeriod object that strings these problems together and respects the requirements that the inputs and outputs of the successive stages mesh with each others. If we wanted to solve an infinite horizon problem, the algorithm would be just to:
Maybe in a finite horizon problem we would want to assume that in the periods from age 90 to 120, the problem is a very simple one, with only stochastic medical shocks (or medical shocks and stochastic pension income). Fine, then the way you do that is you build a constructor that first defines the problem of a 119 year old as just having one stage, which is the solution to a "ConsMedOldOldGuy" problem, and then adds the problem of a 118 year old, which also consists entirely and exclusively of the apparatus necesssary to solve a "ConsMedOldOldGuy" problem, and so on until you get to the 90 year old. Maybe between 90 and 80, there is some utility from other activities, so you perhaps add an extra stage to the problem of those agents: "ConsFairlyOldGuy"; but you continue building back until you get to age 80. Then maybe between 80 and 70 you have more options, like travel, which is the solution to a "ConsYoungOldGuy" problem; so you need to add another stage to the decision problem for those people. Then between 60 and 70 you have people who may or may not have retired, so you have a "retire yes or no" stage, so now there's a "ConsMaybeRetireGuy" problem. And so on. What is an "AgentType" here? Or, what is an "AgentType" in an infinite horizon problem, as distinct from just an agent solving a particular type of problem? Why do we need an AgentType at all? Maybe we need StageTypes because there might be stages in the infinite horizon consumer's problem (portfolio optimization, labor supply, consumption) but that is really about stringing together several solution models in order. |
|
I agree that:
I believe that @mnwhite @alanlujan91 and I have discussed this and have agreed that it would make sense to have a more modular architecture that cleanly separates: Model definitions, Solvers/solutions, Simulators, and maybe Estimators. These should plug into each other with clean APIs but be distinct. I also believe we've agreed that AgentType can, in the future, be a class that wraps these components in a useful way. Rather than holding a lot of functionality inside it, the AgentType can be an object that takes a model, a solve, and a simulator. Essentially, the biggest problem with HARK right now is that because everything is so tightly coupled in the AgentType class with its 'whiteboard problem', and everything that inherits from it, any substantive improvement requires laborious updates to all downstream models. I think it would make sense for @alanlujan91 to prepare a PR with just the Parameters class, standing alone and with tests, but not blocking on the big AgentType refactor. That way the Parameters object could be used in more modular components (like the simulators and solvers I've been working on). I believe @mnwhite has volunteered to be a hero and refactor the AgentType classes? Maybe a lightweight way to start would be to shift over to the Parameters object. If Parameters is already in the library, he could adjust that class if it's necessary to do so when putting it into practice. |
|
This is one of those things where it's not clear where to draw the line
between incrementally improving the way we do things now versus thinking
about a comprehensive redesign for HARK 2.0.
Let's discuss today.
…On Wed, Aug 9, 2023 at 10:01 AM Sebastian Benthall ***@***.***> wrote:
I agree that:
- refactoring all the AgentType models is going to be work
- it's not clear what AgentType is or should be
I believe that @mnwhite <https://github.com/mnwhite> @alanlujan91
<https://github.com/alanlujan91> and I have discussed this and have
agreed that it would make sense to have a more modular architecture that
cleanly separates: Model definitions, Solvers/solutions, Simulators, and
maybe Estimators. These should plug into each other with clean APIs but be
distinct.
I also believe we've agreed that AgentType can, in the future, be a class
that wraps these components in a useful way. Rather than holding a lot of
functionality inside it, the AgentType can be an object that takes a model,
a solve, and a simulator.
Essentially, *the* biggest problem with HARK right now is that because
everything is so tightly coupled in the AgentType class with its
'whiteboard problem', and everything that inherits from it, any substantive
improvement requires laborious updates to all downstream models.
I think it would make sense for @alanlujan91
<https://github.com/alanlujan91> to prepare a PR with *just* the
Parameters class, standing alone and with tests, but not blocking on the
big AgentType refactor. That way the Parameters object could be used in
more modular components (like the simulators and solvers I've been working
on).
I believe @mnwhite <https://github.com/mnwhite> has volunteered to be a
hero and refactor the AgentType classes? Maybe a lightweight way to start
would be to shift over to the Parameters object. If Parameters is already
in the library, he could adjust that class if it's necessary to do so when
putting it into practice.
—
Reply to this email directly, view it on GitHub
<#1240 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK7YM2HTLMHGZJ5BJR2DXUOJ2FANCNFSM6AAAAAAVVP37KU>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
- Chris Carroll
|
|
@llorracc I'm troubled that after all of the recent meetings, you are still drawing a dichotomy between "incremental improvements" and "HARK 2.0". Please consider that we might make incremental improvements to HARK 1.0, and then incrementally improve to HARK 2.0. |
|
No, I get it.
My agenda is just to make sure, when we make the incremental improvements,
that we have thought about how they relate to the "big picture". Often
there might be implications for alternative ways to do the incremental
improvements, some of which would cause trouble downstream and others of
which would not cause trouble. I want to make sure we make wise choices
now in order to save ourselves the time that otherwise will be required in
the future to work around things that would not require a workaround if we
had done them thoughtfully now.
…On Wed, Aug 9, 2023 at 10:19 AM Sebastian Benthall ***@***.***> wrote:
@llorracc <https://github.com/llorracc> I'm troubled that after all of
the recent meetings, you are still drawing a dichotomy between "incremental
improvements" and "HARK 2.0".
Please consider that we might make incremental improvements to HARK 1.0,
and then incrementally improve to HARK 2.0.
—
Reply to this email directly, view it on GitHub
<#1240 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK76T3PHDUCW7H3YCNNLXUOL55ANCNFSM6AAAAAAVVP37KU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
- Chris Carroll
|
|
@Mv77 @sbenthall pending passing of tests, I think this is reached the point where it can stand on its own and be used as needed. If you have time to review this I would appreciate it. |
| class Model: | ||
| """ | ||
| A class with special handling of parameters assignment. | ||
| """ | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
May we consider initializing a parameters object when a model is created, even if it is not used (for now)?
I know re-working all of hark so that it works with this new object is a huge lift, but maybe creating the object in the existing agent types can open the opportunity to start using it.
For instance, if ConsIndShock had a parameters object of this new class (currently a dictionary), we could start reworking bits like the income process (which is pervasive across hark) to use this new tool. Creating it as an extra property would not break the existing code.
The idea would violate the principle of having a single source of truth for any param but I think it'd be a nice way to start working this in. Would be interested in hearing what @sbenthall @mnwhite think.
There was a problem hiding this comment.
Looks good to me,
It's hard for me to ask for features or be critical of the code without it being used in an agent type or to streamline some of the existing methods. We've agreed that's a huge lift that should be postponed, so I am looking at this as the creation of a structure that will be used in the future. It is a nice structure and I'd be happy to have it merged in.
How are we going to tackle the huge lift?
|
This passes my review. Nice work @alanlujan91 . Has the CHANGELOG been updated? |
|
Will do this later today! |
|
Hooray! This will be merged now! |
Please ensure your pull request adheres to the following guidelines: