Rng fixes #428

cliffckerr · 2024-03-31T04:53:44Z

Additional RNG fixes for #426. Also adds an SIS disease class among other fixes.

daniel-klein

Overall a nice set of improvements! A few minor suggestions here already, and I'll continue playing with it and report back should anything meaningful come up.

daniel-klein · 2024-04-01T19:50:00Z

starsim/demographics.py

@@ -344,7 +344,7 @@ def standardize_fertility_data(self):

    def initialize(self, sim):
        super().initialize(sim)
-        low = sim.pars.n_agents+1 # TODO: or 0?
+        low = 0 # Was sim.pars.n_agents + 1


We know collosions will occur if reusing slots from the initial population.

Also, increasing slot_scale previously guaranteed no slot duplicates in the limit, but with this change we would no longer have such a guarantee.

Did some testing (see #436), and agree the previous version had fewer collisions (though still a lot!), with only a tiny performance penalty for slot_scale=5 (~3%). Reverted.

daniel-klein · 2024-04-01T20:39:53Z

starsim/modules.py

+                if val.initialized is not True: # Catches False and 'partial'
+                    val.initialize(module=self, sim=sim, force=True) # Actually a dist
+                else:
+                    print(f'TEMP: tried to reinitialize {val}')


Adjust before final.

daniel-klein · 2024-04-01T20:42:46Z

starsim/network.py

+            if not self.pars:
+                self.pars.n_contacts = 10
+        if 'n_contacts' in self.pars: # Convert from n_contacts to probability
+            self.pars.p = self.pars.pop('n_contacts')/n_agents


Oh, I hadn't realized that n_contacts is normalized by the population size rather than being used as an absolute number. Benefits and drawbacks to each approach, but we should rename the parameter in light of population scaling.

daniel-klein · 2024-04-01T20:44:15Z

starsim/network.py

        self.graph = graph
        self.pars = ss.omerge(pars)
+        self.dist = ss.Dist(distname='StaticNet')


As my first time seeing this code, I don't know what type of Dist will result from this statement. If I were to ask this dist for rvs, would they be normally distributed? Uniform? Etc. I suspect here it's just being used to access the underlying rng and machinery.

Correct, it will give:

ValueError: Dist.rvs() failed: no valid NumPy/SciPy function found in this Dist. Has it been created and initialized correctly?

daniel-klein · 2024-04-01T20:46:38Z

starsim/network.py

-        target = np.random.permutation(source)
+        source = self.get_source(inds, n_contacts)
+        target = self.dist.rng.permutation(source)
+        self.dist.jump() # Reset the RNG manually # TODO, think if there's a better way


I don't believe RandomNet is RNG safe anyway.

It's not, but there's more and less safe versions :)

daniel-klein · 2024-04-01T20:56:03Z

starsim/sim.py

+        self.diseases      = ss.ndict(diseases, type=ss.Disease)
+        self.networks      = ss.ndict(networks, type=ss.Network)
+        self.connectors    = ss.ndict(connectors, type=ss.Connector)
+        self.interventions = ss.ndict(interventions, type=ss.Intervention, strict=False) # strict=False since can be a function


Consider a strict=True default that reverts to False if the user provides a function?

This is not Dist(..., strict=False), this is ss.ndict(..., strict=False), which raises an exception if an object is the "wrong" type. But here we expect some interventions might be function instead of ss.Intervention, so we don't want to be strict. (Will probably refactor soon anyway!)

daniel-klein · 2024-04-01T20:56:38Z

starsim/sim.py

+        self.networks      = ss.ndict(networks, type=ss.Network)
+        self.connectors    = ss.ndict(connectors, type=ss.Connector)
+        self.interventions = ss.ndict(interventions, type=ss.Intervention, strict=False) # strict=False since can be a function
+        self.analyzers     = ss.ndict(analyzers, type=ss.Analyzer, strict=False)


Analyzers are unlikely to be using random numbers anyway, so strict shouldn't really matter. But curious on why False for a default?

(Weird, I thought I already replied to this!) This is strict for ndict, not for Dist. This prevents strict type checking, which caused this to fail if you passed it a function instead of an ss.Analyzer object.

daniel-klein · 2024-04-01T20:58:50Z

starsim/sim.py

@@ -447,9 +463,9 @@ def validate_post_init(self):
        TBC whether we keep this or incorporate the checks into the init methods
        """
        # Make sure that there's a contact network if any diseases are present
-        if self.diseases and not self.networks:
+        if self.diseases and not self.networks: # TODO: handle NCDs?


YES! Many NCDs and even some disese modeling applications will not have networks. It's just a warning, but wouldn't want to give users the impression that something is wrong if this is as intended.

Agree, just removed for now. It should really be a warning as it's trying to process beta.

daniel-klein · 2024-04-01T21:00:28Z

starsim/sim.py

-    kw = sc.mergedicts(dict(pars=dict(diseases='sir', networks='random')), kwargs)
-    sim = Sim(**kw)
+    pars = sc.mergedicts(dict(diseases='sir', networks='random'), kwargs)
+    sim = Sim(pars)


Would like the demo to at least mention the mainstream approach to specifying modules so it doesn't appear as though the string-based approach is the only way.

Personally I think anyone whose sole knowledge of how to run Starsim comes from looking at the demo code probably should be restricted to using string arguments 🙃 But agree that we should make sure the rest of the docs, tests, and examples illustrate the full range of possible usages.

daniel-klein · 2024-04-02T00:13:12Z

tests/test_base.py

@@ -6,7 +6,7 @@
 import sciris as sc
 import numpy as np
 import starsim as ss
-import matplotlib.pyplot as plt
+import pylab as pl


We've had this discussion previously, but I think important matplotlib.pyplot is preferable here. If you browse to Matplotlib's page on pylab here (https://matplotlib.org/stable/api/pylab.html) they say:

pylab is a historic interface and its use is strongly discouraged. The use of pylab is discouraged for the following reasons: from pylab import * imports all the functions from matplotlib.pyplot, numpy, numpy.fft, numpy.linalg, and numpy.random, and some additional functions into the global namespace. Such a pattern is considered bad practice in modern python, as it clutters the global namespace. Even more severely, in the case of pylab, this will overwrite some builtin functions (e.g. the builtin sum will be replaced by numpy.sum), which can lead to unexpected behavior.

Fine† :) I've replaced it with import matplotlib.pyplot as pl, which hopefully will annoy everyone equally :)

† The article seems to conflate import pylab as pl with from pylab import * -- the latter I definitely agree is bad!

daniel-klein · 2024-04-02T17:04:07Z

Wait, just seeing the auto advance in Dist, which seems to circumvent strict as it defaults to True. Auto advancing is safe in some cases, but not others. There are really just a handful of places where auto is needed, like in the new pairwise transmission code. Recommend investigating permanent solutions for these challenge spots rather than auto jumping.

cliffckerr added 30 commits March 30, 2024 11:14

update version and changelog

31cd790

not working, going to revert

fa67f50

reworking initialization, but not working yet

4984ed1

change lognorm_ex inheritance, add debug

4564f4c

made strides toward network CRN safety, but not there yet

1c21a52

small fix to hpvnet

4b3757c

update using breadth first search

d1ccd68

lots of failures when turning strict off

a0efb0b

very unsure of the difference in dists

255393e

undoing sir debugging changes

f2fbfe0

fixed outdated reference to rng

07857dd

reinstate jump

adff772

tidying

85b86ca

add reset test

24f6732

remove enum

36b919f

rename basedemographics

63f67b3

working on syphilis

17c2543

syphilis still odd but ok

51efd2f

tidying tests folder

455d350

rename test_dists to test_distributions

21d62b9

done tidying dist tests

5bab7bc

fix lambda

51f30a9

change slot definition

a9d6b2f

fixes to distributions

1baf711

tests pass but gives warning, need to figure out why

0f364ab

revert changes, unclear why other version failed though

f1d2d7f

add SIS model

18b6005

more updates

b165293

pure numpy version is slower

d25e255

revert to numba version

99ac915

implemented both versions

b24448a

cliffckerr mentioned this pull request Apr 1, 2024

Implement SIS model #424

Closed

cliffckerr added 4 commits April 1, 2024 02:37

update analyzers

a1f851f

fix interventions as functions

160d2eb

allow function analyzers

a9974e4

update changelog

67460e8

cliffckerr requested review from robynstuart, daniel-klein and RomeshA April 1, 2024 07:27

add additional validation logic

9a40f64

cliffckerr mentioned this pull request Apr 1, 2024

Networks are not RNG-safe #342

Closed

cliffckerr added 4 commits April 1, 2024 18:11

working reasonably well

422530e

add additional multisim test

085ad96

update template

5efa705

update function template

f14260d

daniel-klein reviewed Apr 2, 2024

View reviewed changes

cliffckerr added 7 commits April 1, 2024 22:52

add slots back into pregnancy

dd77967

debugging slots

d263e3a

restore slot scale

e94f616

fix error message

cd5ccca

remove no network warning

c8fdab1

replace pylab with pyplot

ce5cdba

warning message improvement

2a79d01

cliffckerr merged commit 2259d62 into main Apr 2, 2024
3 checks passed

cliffckerr deleted the rng-fixes branch April 2, 2024 04:15

cliffckerr mentioned this pull request Apr 2, 2024

Rethink auto-jump in Dist #438

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rng fixes #428

Rng fixes #428

cliffckerr commented Mar 31, 2024 •

edited

daniel-klein left a comment

daniel-klein Apr 1, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 1, 2024

cliffckerr Apr 2, 2024

daniel-klein Apr 2, 2024

cliffckerr Apr 2, 2024

daniel-klein commented Apr 2, 2024

Rng fixes #428

Rng fixes #428

Conversation

cliffckerr commented Mar 31, 2024 • edited

daniel-klein left a 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

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

daniel-klein commented Apr 2, 2024

cliffckerr commented Mar 31, 2024 •

edited