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

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

Conversation

MSallermann
Copy link
Member

No description provided.

MSallermann and others added 2 commits July 28, 2024 15:37
The old type (a plain int with 32 bits) could overflow easily, which
might cause confusion.

Co-authored-by: Amrita Goswami <[email protected]>
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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants