-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
Labeled models #1177
Labeled models #1177
Conversation
It looks like this PR includes a lot of changes that work in your new syntax for utility functions. Can these changes be merged without the additional changes that are about labelled models? It would be easier to review the material in this PR if it was broken up into multiple PRs that focus an particular functions. |
dims=("mNrm"), | ||
attrs={"long_name": "Normalized Market Resources"}, | ||
) | ||
state = Dataset({"mNrm": mNrm}) # only one state var in this model |
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 see you are using a DataArray for an individual variable (?) and a DataSet for an object that bundles all states, actions, etc. (?)
I'm curious why you made this choice. My understanding is that a DataSet is analogous to a tabular database, containing many DataArrays each much like an individual table. The main purpose of a DataSet is to allow joins across its DataArrays. But there will be no just joins across an Agent's states.
Did I get this right? What am I missing?
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 seemed appropriate to me that the state variable is a collection of DataArray's each with it's own attributes, names, etc, which would correspond to a Dataset. In this way, the state variable can be generalized to be an n-dimensional Dataset, each dimension being a DataArray.
def __init__(self, dataset: xr.Dataset, CRRA: float): | ||
|
||
self.dataset = dataset | ||
self.CRRA = CRRA |
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 seems like what you are doing here is making a callable wrapper around an xr.DataSet.
I wonder if this can be implemented in a more general way.
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.
There might be a more general way. This is to use the trick of using the inverse value function to approximate the value function.
# | ||
|
||
# %% | ||
def state_transition(s=None, a=None, params=None): |
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.
Ah, yes. I've found it useful to write transition functions in similar way.
In my code, I've been using x
for states and k
for discretely realized values of shocks.
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.
Here, s=state, a=action(s). Elsewhere I might have used shocks
explicitly, so I should probably be more consistent with this nomenclature.
HARK/distribution.py
Outdated
attrs=None, | ||
var_names=None, | ||
var_attrs=None, | ||
): |
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.
Docstring for this please
HARK/distribution.py
Outdated
return ldd | ||
|
||
@classmethod | ||
def from_dataset(cls, x_obj, pmf): |
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.
Docstring for this please
|
||
# egm requires swap dimensions; make actions and state functions of state | ||
action = xr.Dataset(acted).swap_dims({"aNrm": "mNrm"}) | ||
state = xr.Dataset(state).swap_dims({"aNrm": "mNrm"}) |
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.
You seem to be imputing some semantics on the other order of the dimensions in these DataSets.
What is that semantics?
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 was a suggestion by @llorracc. An action is a consequence of the state, but given the end of period state, the endogenous grid method tells you how the agent must have acted in order to be optimal. This should be made more clear in documentation.
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 is cleverly implemented!
Documentation/CHANGELOG.md
Outdated
@@ -28,6 +28,7 @@ Release Date: TBD | |||
* Adds keyword argument `labels` to `expected()` when using `DiscreteDistributionXRA` to allow for expressive functions that use labeled xarrays. [#1156](https://github.com/econ-ark/HARK/pull/1156) | |||
* Adds a wrapper for [`interpolation.py`](https://github.com/EconForge/interpolation.py) for fast multilinear interpolation. [#1151](https://github.com/econ-ark/HARK/pull/1151) | |||
* Adds support for the calculation of dreivatives in the `interpolation.py` wrappers. [#1157](https://github.com/econ-ark/HARK/pull/1157) | |||
* Creates `UtilityFuncCRRA` which is an object oriented utility function with a coefficient of constant relative risk aversion and includes derivatives and inverses. [#1168](https://github.com/econ-ark/HARK/pull/1168) |
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.
We should merge in this PR #1168 first and then revisit this material around Labeled Models.
Yes, there are changes on this branch that are part of other PRs, but I should probably remove those. The most important files for this PR are |
Codecov ReportBase: 73.32% // Head: 73.26% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1177 +/- ##
==========================================
- Coverage 73.32% 73.26% -0.07%
==========================================
Files 74 76 +2
Lines 12122 12529 +407
==========================================
+ Hits 8889 9179 +290
- Misses 3233 3350 +117
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 at Codecov. |
state_dict[coords] = state[coords] | ||
|
||
result = CRRAutility( | ||
self.dataset["v_inv"].interp( |
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 am confused about what mathematical object "v_inv" refers to.
It does not appear to be the mathematical inverse of the value function, which would be my first guess.
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 the last period,
In this case
We can do the same at earlier periods, where
self, | ||
value: ValueFuncCRRALabeled, | ||
policy: xr.Dataset, | ||
continuation: ValueFuncCRRALabeled, |
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.
To the extent that value
, policy
, and continuation
all are wrappers around an underlying Dataset with shared coordinates, maybe this can be designed more flexibly.
|
||
dataset = xr.Dataset( | ||
{ | ||
"cNrm": cNrm, |
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 it's an interesting choice that this is being labeled with cNrm
, which names a variable, when it refers to a policy or decision rule that chooses the value for this variable based on preceding state variables.
I believe in HARK this was called something like cFunc
in order to denote that it's a decision rule.
I wonder if there's a good reason to pick the more concise notation in this version of the model
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 assumed that was an artifact of differentiating between an array and a callable (function). Since xarray does both, I didn't think i needed to differentiate
value=vfunc, | ||
policy=dataset[["cNrm"]], | ||
continuation=None, | ||
attrs={"m_nrm_min": 0.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.
whereas value
, policy
, and continuation
are all objects at the level of abstraction of the Bellman problem, attrs
is as the level of abstraction of the underlying xarray-powered data structure.
That's somewhat mismatched, and I wonder about the meaning of the `"m_nrm_min" field here. Is this representing a constraint on the policy, or a description of the state grid?
(Also, we should probably use either cNrm
or c_nrm
but not switch between them?)
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.
m_nrm_min
is the minimum allowable market resources, or natural borrowing constraint. I think this can be considered a Bellman property, but not sure how to design it since it's just a lower bound.
return ns | ||
|
||
def reverse_transition(self, ps=None, a=None, params=None): | ||
"""state from post_state and actions""" |
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.
'reverse' or 'inverse' ?
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 reverse is more appropriate. State transition is forward (state to post-state, given action), reverse transition is backward (post-state to state, given action).
variables.update(ps) | ||
|
||
variables["reward"] = self.u(action["cNrm"]) | ||
variables["v"] = variables["reward"] + params.Discount * continuation(ps) |
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 may be my own ignorance, but the question of whether the application of params.Discount
should be here or in the continuation (and whether other discounting bits like the PermGroFac term should be here or in the continuation) is confusing to me.
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.
For this piece, I wanted to consider Discount
as a coefficient (float) parameter. Stochastic discounts go into the calculation of the continuation function.
@llorracc Further changes can be made to these models, but I think they are in good enough shape for release 0.13, pending passing checks |
Hi @alanlujan91 I know you are stoked to get this feature in before the 0.13 release. We use the numpy standard for docstrings so API documentation is autogenerated into ReadTheDocs: For the tests, they can probably be copied and edited from the existing consindshock tests, and used to verify that the new implementation is bug free. Given that the 0.13 release is already way overdue, I would recommend that we let you give this PR the attention it deserves and hold off on merging it until later. I would like to get on a semiannual release cycle for HARK, which would mean that we would have another release this coming summer. |
@sbenthall, while I am in agreement with your desire to have a regular release schedule and twice a year sounds right, I'm really keen to get this into the 0.13 release -- so much so that I'd propose that we wait until this is ready before making the 0.13 release. |
Very well. We can hold off on the 0.13 release. But we need tests and docs for this work. |
I agree, we need to wait for tests and docs. |
minimal testing for labeled models
Merged. Hooray! Great work @alanlujan91 |
Please ensure your pull request adheres to the following guidelines: