-
Notifications
You must be signed in to change notification settings - Fork 3
Add shock discretization functionality #206
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
base: main
Are you sure you want to change the base?
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
Very nice! I have a few general comments. Excited to discuss this soon! 🙂
Bugs
- ShockGrid not exported - Listed in all but not imported in src/lcm/init.py
API Design
- Inconsistent pass pattern - Built-in shock transitions use
def next_x(): passwhile all other transitions return values. Consider inferring transitions automatically from ShockGrid - if a state has a ShockGrid, generate its transition implicitly (no next_ function needed). This also removes the redundant distribution_type in both grid and decorator. Only custom shocks would need a next function then. Counter argument would be that this breaks the "contract" that each state variable (which a shock is) has a next function. - ShockGrid.get_coordinate breaks interface - Takes extra params argument unlike other ContinuousGrid subclasses (hence the # ty: ignore). Could consider a different abstraction?
Implementation
- ShockGrid.to_jax() returns placeholders - Returns [0, 1, 2, ...] until update_sas_with_shocks fills in real values. This implicit contract could cause subtle bugs.
- InternalRegime now mutable - Changed from frozen=True to frozen=False. Could use dataclasses.replace() instead to preserve immutability?
- Python loops in discretization - _fill_tauchen and rouwenhorst use Python loops. Could vectorize with jax.vmap?
Minor
- Outdated docstring - ShockGrid docstring mentions nonexistent start/stop attributes
|
Plan:
|
timmens
left a comment
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 starting to look really really good!
for more information, see https://pre-commit.ci
|
I think I was able to make most of the requested changes. There is a new I also tested the solution with the MY Model and there are only extremely small differences during the solution, probably caused by the use of When creating the tests for the shocks, I found a bug with the RNG-Keys used during the simulation. For some reason, the names of the |
hmgaudecker
left a comment
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 coming along very nicely, thanks a lot!!! I only looked at the interface so far. Looks neat, though a bit more streamlining could be helpful (maybe documentation is enough).
I also like the fixed_params attribute as a start, we'll flesh it out elsewhere then.
Thanks for tracking down those bugs, they sound like they would have become a nightmare!
| income_grid = create_income_grid(income_process) # ty: ignore[invalid-argument-type] | ||
| chimax_grid = create_chimaxgrid(chi) | ||
| xvalues, xtrans = rouwenhorst(rho, jnp.sqrt(income_process["sigx"]), 5) # ty: ignore[invalid-argument-type] | ||
| prod_shock = 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 am a little lost with the naming (productivity_shock and prod_shock) and structure here. If I understand correctly,
productivity_shocksets the grid and vianext_productivity_shockits transitionprod_shockis only for setting the initial distribution?
However, why do we need to repeat the distribution_type, n_points, and shock_params arguments? Wouldn't an interface Shock(shock_grid, shock_transition) be doable? Thinking DRY here.
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 mostly caused by the new fixed_param. The ShockGrid can only be fully initialized when the Model gets created, because it's missing the params. The only other way to do this would be to repeat the params, eg when creating the ShockGrid or the Shock and then in the fixed params. Right now the Shock Class is therefore only a streamlined way to access the internal shock discretization functions. Therefore the repetition can at least be avoided if one does not need the Shock outside the model.
The next_state function would also optimally be eliminated for Shock states, but this currently clashes with the way we process transitions. This should be part of a different PR that eliminates the duplication of all next_state functions for all regimes.
This PR is duplicate of #170. Merging the previous PR with the main branch became unfeasible because there were too many merge conflicts.
This PR will add a way to specify multiple common shock types directly through the interface of LCM.
To accomplish this, a new
ShockGridgrid class will be added. The parameters for the shocks are passed via a newfixed_parametersattribute during the model creation. During the solution the shocks will be treated like normal DiscreteGrids, but during the simulation, the next values will be drawn from the shocks undiscretized probability distribution. Here the shocks transition probabilities will be linearly interpolated, as the discretized transition probabilities are only calculated at the gridpoints, but subjects can land between gridpoints.