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

IndexDstn rework #1104

Merged
merged 3 commits into from
Jan 20, 2022
Merged

IndexDstn rework #1104

merged 3 commits into from
Jan 20, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jan 19, 2022

The current implementation of IndexDstn makes its random-number-generation work slightly different to what the majority of the HARK code currently does. Find the details in the discussion in #1024.

There is no right or wrong way to handle the RNG. Some might prefer what IndexDstn does, others might prefer what the legacy HARK code does. In the end both ways of doing things
a) Represent the same distributions when solving the agents' problems.
b) Draw shocks that follow the same (correct) distributions.

The issue is that the exact value of individual shocks is altered when one tries to replace legacy HARK code with the newer IndexDstn tool, and this breaks simulation tests.

This PR changes IndexDstn so that the user has the option of creating a new RNG (current behavior) or use an existing one (legacy HARK tests) to draw the seeds of the time-specific distributions that compose the IndexDstn.

This (I hope) will allow me to use IndexDstn and still pass tests. Eventually, if one random-number-generation method is favored over the others, we can go back and alter the tests accordingly (being sure that changes come only from RNG).

Please ensure your pull request adheres to the following guidelines:

  • 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.

@Mv77
Copy link
Contributor Author

Mv77 commented Jan 20, 2022

The PR also introduces a slight change in how the surrogate distributions are stored.

Previously, MyIndexDstn[n] would call the constructor with the nth set of parameters each time it was called.

With this change, the distributions with all sets of parameters are created at __init__ and then MyIndexDstn[n] returns self.dstns[n]

@sbenthall
Copy link
Contributor

This looks good and I'll merge it.

I do want to say that doing this hack to keep the code in sync with the numerical simulation tests, instead of correcting the numerical simulation tests to be functional, is kicking the can down the road.

The RNG object actually has the power to simulate all the distributions that we need natively; rather than passing the RNG to a Distribution class, we could possibly just be calling the RNG's own distribution methods:
https://numpy.org/doc/stable/reference/random/generator.html#distributions

But we won't be able to do this until the simulation tests are functional instead of tied to specific numerical values.

@sbenthall sbenthall merged commit 84ee03e into econ-ark:master Jan 20, 2022
@Mv77 Mv77 deleted the plumbing/index_dstn_rework branch January 24, 2022 15:13
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
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.

2 participants