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

set seed on distribution initialization, not draw. #691

Merged
merged 13 commits into from
Jul 2, 2020

Conversation

sbenthall
Copy link
Contributor

fixes #619

@sbenthall
Copy link
Contributor Author

It looks like Windows handles maximum integer length differently from MacOS and Ubuntu.
My understanding was that with Python 3, ints are of unbounded length.
But on Windows it looks like they are still int32.

I suppose it's worth articulating what I'm trying to do in the failing code...

HARK seems to be designed so that a model performs identically, though pseudorandomly, when ran twice. That's done through default random seeds.

Before, the seeds were passed to the distribution code when the distribution was drawn from.
The change here is to give a distribution a seed when it's initialized.
That means that the draws from a distribution won't by default have identical values.

One issue raised by this new design is whether and how to the seeds from a continuous distribution to the new distribution created by its discretization.

In this code, it's drawing that seed pseudorandomly from the continuous distribution's generator.

@sbenthall
Copy link
Contributor Author

Debugging this is hard because I can't run the WIndow build natively.

But I'm trying these fixes, in light of the fact that RandomState seeds must be convertible to unsigned 32-bit integers:

  • explicitly setting 'int32' dtype on the integer generators within the distribution code
  • setting the maximum at 2**31-1 because one bit is used for the positive/negative sign, so when converted to an unsigned integer the bit will be redundant...

See: numpy/numpy#4085

@mnwhite
Copy link
Contributor

mnwhite commented May 21, 2020

Add quick reset() method to set RandomState (to same value).

@sbenthall
Copy link
Contributor Author

When does a reset() method need to be called by the AgentType class?

Currently, most distributions used in solution and simulation are constructed just before they are used, then discarded.

I would like to eventually change that so that the distributions are provided when the model is initialized, but things don't work like that yet.

@mnwhite
Copy link
Contributor

mnwhite commented May 22, 2020 via email

@sbenthall
Copy link
Contributor Author

reset() method added in last commit

@mnwhite
Copy link
Contributor

mnwhite commented May 28, 2020

Should we merge this?

@mnwhite
Copy link
Contributor

mnwhite commented May 28, 2020

Ugh, we'll have to adjust the AggShock economy test targets, as this PR changes the random seed. The targets change because the tests don't actually solve the model, but instead quit after 3 passes; the idiosyncratic shocks thus matter.

@mnwhite
Copy link
Contributor

mnwhite commented Jul 2, 2020

I thought I merged this. Pretty sure it's ready for merge after these checks run.

@mnwhite mnwhite merged commit 3bdfb09 into econ-ark:master Jul 2, 2020
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.

discrete distributions should get a random seed on initialization
3 participants