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

Fixed random seed generator to generate unsigned integers instead of … #577

Merged
merged 32 commits into from
Dec 10, 2022

Conversation

JoeJimFlood
Copy link
Contributor

The value of _MAX_SEED in random.py is currently set to the maximum 32-bit unsigned integer, but the default data type for np.random.RandomState().randint() is at 32-bit signed integer. This was causing an error when the default random seed was set to be None and triggering the random seed generation.

To test this, I ran the SANDAG 1-zone examples twice and the 3-zone example once, all of which were set to use random seeds. All runs completed successfully. I compared the mode share from the two 1-zone runs. They weren't exactly the same but at most were within 0.3%, which is what I would expect if the random seed generator worked correctly.

@coveralls
Copy link

coveralls commented Aug 3, 2022

Coverage Status

Coverage decreased (-1.4%) to 55.068% when pulling 90f073d on JoeJimFlood:fix_gen_random_seed into 3df695b on ActivitySim:master.

@jpn-- jpn-- changed the base branch from main to develop August 25, 2022 21:16
@jpn--
Copy link
Member

jpn-- commented Aug 29, 2022

@JoeJimFlood do you want to take a stab at writing a test to run in our continuous integration testing, that confirms this change works as expected? I envision one easy way would be setting the seed to None on to an existing test example and running to check that the final results to NOT exactly match the "expected" results with a zero seed.

@JoeJimFlood
Copy link
Contributor Author

@jpn-- I've never done that before but I'd love to try. Would I just need to edit the settings.yaml file in the test subdirectory of one of the examples? For example in this file?

@JoeJimFlood
Copy link
Contributor Author

@jpn-- I had opened this pull request before Version 1.1 was released. Would it be better if I closed it and created a new one with the same changes applied to the v1.1 code?

@dhensle dhensle self-requested a review October 20, 2022 16:39
@JoeJimFlood
Copy link
Contributor Author

I added a test of the random number generator to the tests in actions. To test that I ran it using a version of the software that has that test but not the random generator bug fix, meaning that the random seed generator test should fail due to the seed always being zero. The results of that are here, where the random seed test failed and all the others passed. I guess that would mean...
task_failed_successfully

activitysim/test/random_seed/configs/settings.yaml Outdated Show resolved Hide resolved
activitysim/test/random_seed/test_random_seed.py Outdated Show resolved Hide resolved
activitysim/test/random_seed/test_random_seed.py Outdated Show resolved Hide resolved
activitysim/test/random_seed/test_random_seed.py Outdated Show resolved Hide resolved
HOW_TO_RELEASE.md Outdated Show resolved Hide resolved
@dhensle
Copy link
Contributor

dhensle commented Dec 3, 2022

Looks good to me, thanks Joe!

@jpn-- jpn-- merged commit d137b36 into ActivitySim:develop Dec 10, 2022
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.

4 participants