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

Module exports #332

Closed
wants to merge 8 commits into from
Closed

Module exports #332

wants to merge 8 commits into from

Conversation

robynstuart
Copy link
Contributor

@robynstuart robynstuart commented Feb 28, 2024

WIP attempt to export modules and treat parameters consistently

Would fix #301

@@ -167,3 +168,43 @@ def create(cls, name, *args, **kwargs):
return subcls(*args, **kwargs)
else:
raise KeyError(f'Module "{name}" did not match any known Starsim Modules')

def _store_args(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adapting logic from HPVsim for exporting interventions, but perhaps there would be a nicer way?

@@ -81,6 +81,68 @@ def update_pars(self, pars=None, create=False, **kwargs):
self.update(pars)
return

def init_module_pars(self, mpar, mtype):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this from sim to parameters, it could also go in the modules themselves as a class method

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I kind of like that actually! (having it be in modules)

@@ -23,13 +23,11 @@ def set_numba_seed(value):

class Sim(sc.prettyobj):

def __init__(self, pars=None, label=None, people=None, demographics=None, diseases=None, networks=None,
connectors=None, interventions=None, analyzers=None, **kwargs):
def __init__(self, pars=None, label=None, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep going back and forth on this. If pars was named specs or something, it would seem very logical to have all the modules be treated as inputs. And since we want to enable people to flexibly enter whatever they want in the pars dict, that seems like the direction we're heading. But if that's the case, it doesn't make sense to me to have them separately listed in the sim inputs

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I do like having these be explicit, but I think they should be treated the same in both cases

@@ -43,19 +41,28 @@ def __init__(self, pars=None, label=None, people=None, demographics=None, diseas
self.tivec = None
self.npts = None

# Define accepted types of module
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't have to go here, but i do think we need a place where we store the list of modules that our sim knows how to interpret. We're implicitly limiting this anyway by only calling initialization methods on the modules that we understand, but how do developers/users know what types we accept?

Also, need to figure out products!

@robynstuart robynstuart marked this pull request as ready for review February 28, 2024 05:06
Copy link
Contributor

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

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

Overall I think this is an improvement, I do have some questions about implementation though

@@ -23,13 +23,11 @@ def set_numba_seed(value):

class Sim(sc.prettyobj):

def __init__(self, pars=None, label=None, people=None, demographics=None, diseases=None, networks=None,
connectors=None, interventions=None, analyzers=None, **kwargs):
def __init__(self, pars=None, label=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I do like having these be explicit, but I think they should be treated the same in both cases

Comment on lines +266 to +269
self.pars.demographics += background_deaths

# Iterate over demographic modules and initialize them
for dem_mod in demographics:
for dem_mod in self.demographics.values():
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between self.pars.demographics and self.demographics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question - what do we think it should be?

For a while I thought that sim.pars should be an exact copy of what users entered, while the sim attributes should be whatever we created based on what was in the pars dict. Sometimes this would be the same, if users entered modules directly, but other times you'd have sim.pars.demographics = True and sim.demographics would then be [ss.Births(), ss.Deaths].

That isn't how it is currently nor how this PR does it, but it makes sense to me to do it that way. Have to be mindful of what we expect the pars to be able to do - e.g., be exportable, be able to recreate a simulation(?), be modifiable.

self.analyzers = ss.ndict(analyzers, type=ss.Analyzer)
module_kwargs = {k:ss.ndict(type=v) for k, v in self.module_types.items()}
self.pars = ss.make_pars(**module_kwargs)
self.pars.update_pars(sc.mergedicts(pars, kwargs), module_types=self.module_types) # Update the parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love passing module_types this way ... would prefer for the needed logic to be within a sim method

@@ -81,6 +81,68 @@ def update_pars(self, pars=None, create=False, **kwargs):
self.update(pars)
return

def init_module_pars(self, mpar, mtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I kind of like that actually! (having it be in modules)

@@ -23,6 +23,7 @@ class Parameters(sc.objdict):
def __init__(self, **kwargs):

# Population parameters
self.people = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember, do we also support ss.Sim(pars=dict(people=dict(n_agents=10e3, ...))) -- which I'm not sure we do currently? (i.e. have a people pars dict in addition to being able to supply people pars directly in the sim pars)

@robynstuart robynstuart marked this pull request as draft February 29, 2024 00:27
@cliffckerr
Copy link
Contributor

Closed by #488

@cliffckerr cliffckerr closed this May 20, 2024
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.

Rethink Starsim parameters and API
2 participants