New distributions code moved down into emod-api from emodpy (Fix #50, was issue 43 in emodpy previously)#50
Conversation
…structor call (Fix: emod-api-old/685)
…file() demog, updated tests
…moving migration.from_params().
| from emod_api import schema_to_class as s2c | ||
|
|
||
|
|
||
| class BaseDistribution(ABC): |
There was a problem hiding this comment.
Is there a specific reason to put the distribution classes into separate .py file? I would prefer to merge them into a single file so user can import them more simply:
from emod_api.utils.distributions import BimodalDistribution
Not:
from emod_api.utils.distributions.bimodal_distribution import BimodalDistribution
There was a problem hiding this comment.
One class per file aids in readability and more easily understanding where things are by glancing at file lists. I find this to be highly important. For us (me? :D) and for users, especially since these distributions will be user-facing.
Most users will end up using a small subset at any one time, so their imports will not look so detailed.
There was a problem hiding this comment.
Does this break what we did in emodpy-hiv? This seems opposite from our 2.0 pattern for interventions and targeting config.
There was a problem hiding this comment.
This will require a small update in emodpy-hiv , yes.
There was a problem hiding this comment.
will it break user code? We really don't want to break anything in emodpy-hiv.
There was a problem hiding this comment.
Since emodpy needs to be updated, too, I propose that the emod-api distribution classes be imported into init.py in emodpy.utils.distributions. This will avoid breakage in code.
In fact, this is currently what I have in emodpy for PR'ing.
…ject for config consistencty
Merging from mercury, after merging from main, to obtain s2c updates from main.
Bridenbecker
left a comment
There was a problem hiding this comment.
Looks good. Most of my comments are about adding comments and creating tickets for changes to emodpy-malaria and emodpy-hiv.
| default_node: (Node) Represents default values for all nodes, unless overridden on a per-node basis. | ||
| metadata: (Dict) set the demographics metadata to the supplied dictionary. Default yields default | ||
| metadata values. | ||
| set_defaults: (bool) Whether to set default node attributes on the default node. Defaults to True. |
There was a problem hiding this comment.
Shouldn't the documentation for the init() method go above it since it is part of creating an object of the class?
There was a problem hiding this comment.
Uh, I have never put such documentation there. I can put it wherever. It would be good to talk about a team-standard and decide (moving forward). As for here, after taking a look at the below, I'll do whatever.
PEP #257 apparently indicates init() is the "proper" place for this. It is the "most liked" solution in this relevant stackoverflow discussion, but by not means the only solution proposed/discussed.
…t constructor. Changing default idref for demographic files to default_id_reference .
… hiv or malaria sims
…ine printing, numbers indicate nesting depth in space count.
… Appears to work either way.
… of PropertiesAndAttributes
Bridenbecker
left a comment
There was a problem hiding this comment.
Thank you for making all of those changes. the comments about what is used in what disease are really going to be helpful later.
| mortality_distribution_female: MortalityDistribution = None, | ||
| innate_immune_distribution_flag: int = None, | ||
| innate_immune_distribution1: int = None, | ||
| innate_immune_distribution2: int = None |
There was a problem hiding this comment.
My reason for doing the subclassing is that when we support multiple diseases, it helps to the user and developer to know what is supported in what disease.
|
|
||
| # Ensure we have the right config items set now. | ||
| self.assertEqual(self.config.parameters.Age_Initialization_Distribution_Type, "DISTRIBUTION_SIMPLE") | ||
| self.assertEqual(self.config.parameters.Susceptibility_Initialization_Distribution_Type, "DISTRIBUTION_COMPLEX") |
There was a problem hiding this comment.
I guess it depends on whether implicit function for Susceptibility_Initialization_Distribution_Type cascades up.
| input_filename = os.path.join(manifest.demo_folder, "Namawala_four_node_demographics_for_Thomas.json") | ||
| output_filename = os.path.join(self.out_folder, "Namawala_four_node_demographics_for_Thomas_comparison.json") | ||
|
|
||
| self.pass_through_test(input_filename, output_filename) |
…ither (age, susceptibility) will now throw an exception to prevent ambiguity when a demographics file is being created.
|
@Bridenbecker |
Updating after merging main into mercury, closing PR 43 after this if tests pass.
There were a lot of things using the old distributions. Changes in here include, but my memory may (momentarily) be forgetting more: