Skip to content
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

[WIP] Consistent t-indexing for time varying parameters #1027

Closed
wants to merge 14 commits into from

Conversation

sbenthall
Copy link
Contributor

This PR works towards addressing #1022, and along with with #326.

The main change to the core logic is that when a model with time-varying parameters of length T is given to solve_one_cycle, the solutions in the cycle are solved backwards and given inputs drawn from the following state, which is (t+1).

For truly cyclic models, T solutions are created. The T'th solution (which is solved first) draws is parameters from the 0th period.

For "lifecycle" models, where the "cycle" represents a series of steps executed only once, (T - 1) solutions are created. The Tth solution (which is solved first), which corresponds to the _pen_ultimate period, draws parameters from the last period (whose terminal solution is computed elsewhere).

Further changes to the library entailed by this change include:

  • Adding 0th period values to all of the default parameters lists that are non-cyclical.
    • This includes the Calibration/IncomeTools lists. I ask @Mv77 to review my changes here to see if they make sense.
  • "Rotating" all of the cyclical time-varying parameter lists "forward" one step.
  • Removing the special newborn handling code from ConsIndShockModel (see [WIP] cleaning up strange newborn handling in ConsIndShock model #1021 and Default income shock for newborns #326 ) get_shocks(). Because this newborn code created an extra draw() for every agent, changing the distribution's random seed accordingly, changing this will change all numerical values from simulations. So this will require a thorough code review. I request that from @mnwhite

What I should not be getting, which I hope to correct for through further work on this PR, are changes to the solution objects.

Some open questions on my end are:

  • How should the ConsMarkovModel be adjusted to handle this new schema? I see a failing test for a structural (non-numeric) reason but I'm not sure how
  • Why am I getting a failing test for the ConsIndShockModelFast? Maybe @alanlujan91 can tell me

If at all possible, here is what I'm hoping NOT to do in this PR:

  • Resolve the question of how time-varying parameters that are not "look forward" parameters are passed to the solver. I don't think anything I'm doing here is making that harder. But it sounds like that requires a lot of model-specific knowledge.
  • Touching any of the handling of the terminal and pseudo-terminal solutions.

While there are models that may require changes along these lines, I'm not sure any of the existing models do. I would recommend not trying to support a feature that does not have a model and test using it in the library.

  • Tests for new functionality/models or Tests to reproduce the bug-fix in code.
  • Updated documentation of features that add new functionality.
  • Update CHANGELOG.md with major/minor changes.

@sbenthall
Copy link
Contributor Author

Some of the failing tests were due to problems resolved by #1029

I'll do a pass through the remaining test failures trying to fix anything which I think isn't due to the changing newborn handling.

@sbenthall
Copy link
Contributor Author

This PR got messy. I'm going to start again with a clean cut off master

@sbenthall
Copy link
Contributor Author

Nevermind I've found where some code that might be responsible for the failing tests is.

Related to #981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant