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.

Sign up for GitHub

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

Jump to bottom

IO: change type of rng_seed to int64 and added test for network_to_file #50

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

IO: change type of rng_seed to int64 and added test for network_to_file #50

wants to merge 2 commits into from

Conversation

MSallermann
Copy link
Member

No description provided.

MSallermann and others added 2 commits July 28, 2024 15:37
@MSallermann @amritagos
config: change type of rng_seed to int64_t
8df34cd 
The old type (a plain int with 32 bits) could overflow easily, which
might cause confusion.

Co-authored-by: Amrita Goswami <amrita16thaug646@gmail.com>
@MSallermann
test_io: test network_to_file
f9c9e0b 
Also added a constructor to Network that just takes lvalue refs to the
weight and neighbour list.
TODO: Should probably look to reduce code duplication there ...
@User-DK
Copy link
Contributor

User-DK commented Jul 29, 2024 •
edited
Loading

Hello sir,
I would like to know about how this affected the simulation?
Also we, Rohit sir and me recently saw this as I also used it in the bindings, the random_device()() result type is unsigned int then again is using int64 a better choice or can we just use auto ?
image

@MSallermann
Copy link
Member Author

Hi Daivik,

I don't think this had any influence on the simulations.

I think this was more of a cosmetic issue, since it's confusing for the user if the input config.toml file contains a large number as a string (large enough to make the internal representation as an int32 overflow) and, due to that, the logs contain a different seed than was entered in the input file. Effectively, the same seed in the input file (overflow or not) would still lead to the same simulation behaviour.

Regarding the choice of signed vs unsigned: Yeah, it would probably make sense to choose a uint64 as the internal representation of the rng_seed (which is the result type of std::mt19937_64 and would also, of course, cover std::mt19937). I am not sure the TOML spec defines unsigned integers though and how the library we use for parsing toml files behaves in the extremal cases. But yeah, good point and worth considering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amritagos amritagos Awaiting requested review from amritagos amritagos is a code owner

@HaoZeke HaoZeke Awaiting requested review from HaoZeke HaoZeke is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MSallermann @User-DK