model: Ensure the seed is initialized with current timestamp when it is None #1814

rht · 2023-09-20T13:39:37Z

This is a precursor to #1812, and is useful info for debugging purpose, because apparently there is no way to get a random.Random() object's seed once it is initialized. At best, you can have its state instead.

codecov · 2023-09-20T13:41:35Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.08% ⚠️

Comparison is base (3dbabfe) 81.36% compared to head (74913c2) 81.29%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1814      +/-   ##
==========================================
- Coverage   81.36%   81.29%   -0.08%     
==========================================
  Files          15       15              
  Lines         891      893       +2     
  Branches      191      170      -21     
==========================================
+ Hits          725      726       +1     
- Misses        142      143       +1     
  Partials       24       24

Files Changed	Coverage Δ
mesa/model.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Corvince · 2023-09-21T08:22:30Z

mesa/model.py

-        obj._seed = kwargs.get("seed", None)
+        obj._seed = kwargs.get("seed")
+        if obj._seed is None:
+            # See https://docs.python.org/3/library/random.html#random.seed


The comment is not 100% accurate, but I think it can be removed completely anyway. I don't think the rational is important here (it should go into the commit message if you want to preserve it).

I can make it more concise. But I think a new rule of thumb is that if ChatGPT can't deduce the purpose of the code that matches this comment, then the comment is needed.

Also, having to view the blame for a small helpful comment is inconvenient.

Corvince · 2023-09-21T08:24:02Z

mesa/model.py

+            # datetime.now().timestamp() is more cross-platform than using
+            # time.time(), and guarantees that the precision is to the
+            # microsecond.
+            current_timestamp = datetime.datetime.now(datetime.timezone.utc).timestamp()


I don't understand why we need that additional precision here - time.time seems to be simpler and should in any case be sufficient for just having a known seed.

See https://stackoverflow.com/questions/27276135/python-random-system-time-seed#comment134270695_27276285.

Well at the beginning of https://docs.python.org/3/library/time.html#module-time it says that time() will always return the most accurate time available. There is simply no logical way for datetime to achieve a higher precision. This might have been different in the past, the answer is from 2014

The answer is from 2014, but that comment is from 2023.

From https://docs.python.org/3/library/time.html#time.time

Note that even though the time is always returned as a floating point number, not all systems provide time with a better precision than 1 second. While this function normally returns non-decreasing values, it can return a lower value than a previous call if the system clock has been set back between the two calls.

Yes, but if the operating system doesn't provide a better accuracy, how should datetime have a higher precision?

Why did you assume that the OS can't provide higher precision with datetime.now()? It's just that time.time() and datetime.now() may output different precision, depending on the system. Don't just conclude on your own whim that your assumption of the inner workings of time.time() and datetime.now() were correct in the first place.

Well I worked a lot with datetimes and once with an embedded system that had no subsecond precision. Not at the same time, but I think it's fine to say my understanding is a bit more than a "whim". Plus,as I quoted, it says in front of the time docs that time() uses the best precision available.

But I definitely agree that we can quit this conversation now. I just wanted to help you understand.

https://docs.python.org/3/library/datetime.html#datetime.datetime.now

If optional argument tz is None or not specified, this is like today(), but, if possible, supplies more precision than can be gotten from going through a time.time() timestamp (for example, this may be possible on platforms supplying the C gettimeofday() function).

Is this not clear enough?

I'm fine with moving on. I didn't see your last message before my comment.

Please let's talk considerately to each other. I am going to resolve this conversation

Corvince · 2023-09-21T08:25:53Z

I approved, but added 2 comments. That should mean you can make suggested or leave it as is if you strongly disagree. Its not important.

I haven't merged yet, because I don't know if this should go into the 2.1.2. release (which would need to be updated - might be easier to merge later)

Corvince · 2023-09-21T09:07:36Z

mesa/model.py

+        if obj._seed is None:
+            # We explicitly specify the seed here so that we know its value in
+            # advance. This value matches random.Random's default seed, which
+            # is the current timestamp. Note: datetime.now().timestamp() is


It usually isn't the default seed.

If a is omitted or None, the current system time is used. If randomness sources are provided by the operating system, they are used instead of the system time (see the os.urandom() function for details on availability).

Its hard to find a OS without a randomness source.

I have removed the statement that it the timestamp is CPython's default. This is how they do it: https://github.com/python/cpython/blob/d4cea794a7b9b745817d2bd982d35412aef04710/Modules/_randommodule.c#L283. There is a PyObject interface to it, but it is a non-public module, _random.Random.

os.urandom() is a cryptographically secure pseudorandom number generator, and is safer in any cases. Overkill for a simulation, but maybe I could replace the timestamp with os.urandom(624) instead. Do you agree with os.urandom?

The number 624 is taken from its use in https://github.com/python/cpython/blob/d4cea794a7b9b745817d2bd982d35412aef04710/Modules/_randommodule.c#L250.

I would say simply random.random() would be the appropriate choice. That also wouldn't require any comment, because the code itself would already "say": just use a random value. And that's what we want. If someone requires cryptographically secure seeds they can provide them. But I can't really imagine a use case for that.

…is None

rht · 2023-09-21T10:56:54Z

mesa/model.py

+        if obj._seed is None:
+            # We explicitly specify the seed here so that we know its value in
+            # advance.
+            obj._seed = random.random()  # noqa: S311


Had to add the ignore, otherwise Ruff said

S311 Standard pseudo-random generators are not suitable for cryptographic purposes

rht force-pushed the solara-seed branch from 05caedc to 109cfe9 Compare September 20, 2023 14:07

Corvince approved these changes Sep 21, 2023

View reviewed changes

rht force-pushed the solara-seed branch from 109cfe9 to b45569f Compare September 21, 2023 08:43

Corvince reviewed Sep 21, 2023

View reviewed changes

rht force-pushed the solara-seed branch 2 times, most recently from f46f78c to 0e68333 Compare September 21, 2023 10:50

model: Ensure the seed is initialized with current timestamp when it …

74913c2

…is None

rht force-pushed the solara-seed branch from 0e68333 to 74913c2 Compare September 21, 2023 10:56

rht commented Sep 21, 2023

View reviewed changes

Corvince merged commit 6a39efd into projectmesa:main Sep 26, 2023

rht deleted the solara-seed branch September 26, 2023 13:03

rht mentioned this pull request Sep 28, 2023

Solara: Let users rewind sessions #1805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model: Ensure the seed is initialized with current timestamp when it is None #1814

model: Ensure the seed is initialized with current timestamp when it is None #1814

rht commented Sep 20, 2023

codecov bot commented Sep 20, 2023 •

edited

Loading

Corvince Sep 21, 2023

rht Sep 21, 2023

rht Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

rht Sep 21, 2023

tpike3 Sep 21, 2023

Corvince commented Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

rht Sep 21, 2023

Corvince Sep 21, 2023

rht Sep 21, 2023

model: Ensure the seed is initialized with current timestamp when it is None #1814

model: Ensure the seed is initialized with current timestamp when it is None #1814

Conversation

rht commented Sep 20, 2023

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Corvince commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2023 •

edited

Loading