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

Rng warnings #1356

Merged
merged 7 commits into from
Jun 15, 2023
Merged

Rng warnings #1356

merged 7 commits into from
Jun 15, 2023

Conversation

rowleya
Copy link
Member

@rowleya rowleya commented Jun 15, 2023

Ensures that if RNG is used in the wrong place, the user knows about it. This comes as exceptions by default, with an option to turn it into a warning if wanted.

Note that the exception/warning is circumstantial: if the exact parameters mean that things will be generated on machine, but the user has supplied a seeded RNG to places it will be ignored, they are told about it.

All locations in the current code have been fixed in tests and examples. The following have also been changed to match:
SpiNNakerManchester/PyNNExamples#105
SpiNNakerManchester/IntroLab#47
SpiNNakerManchester/microcircuit_model#33

Tested with:
SpiNNakerManchester/IntegrationTests#210

Copy link
Contributor

@andrewgait andrewgait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a comment that you can take or leave...

@@ -104,6 +108,40 @@ def _prod(iterable):
return reduce(operator.mul, iterable, 1)


def _all_gen(rd):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both this and the following function seem like they should be somewhere more general (utility_calls perhaps?) than in the population vertex. I don't mind them being here for now but perhaps consider moving them somewhere else in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, could be moved if needed in future; currently they are only ever used here...

@rowleya rowleya merged commit a0c24a3 into master Jun 15, 2023
@rowleya rowleya deleted the rng_warnings branch June 15, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants