Vector Migration by Genes. Now rebased off of latest. #64
Conversation
…ave to call it many times in MigrationTest) and this way it's called automatically when needed. Although it's only needed for individual vector migration.
…now picking a new index per cohort vs per Queue for migration
… files "for all", "by sex" and "by genes"
…nnoying to type out in cmdline
…netics files as I've created a separate script to create vector migration files
There was a problem hiding this comment.
Thank you for your hard work on this and for persevering through this complicated stuff.
The big thing to think about here is limiting the knowledge between objects. The more objects know about each other, the more coupled they become. This can make them difficult to test, reuse, and modify.
Because our emodpy-hiv schedule is borked, we might as well make these changes as well as change the metadata to JsonConfigurable.
BTW. Don't forget about using const reference when you can.
…9940, 0.999999940, 0.999999940, 0.999999940, 1] and that means there's a chance that things will migrate to the last index if somehow between 0.999999940 and 1 (I had similar issues with fractions migrating because we canculated things backwards from rcd and a very tiny difference ended up with dozens of vectors migrating to a node with migration 0). Now it's [0.333333, 0.666666627, 1, 1, 1, 1, 1, 1]
…is why we need it this way.
… resolving the floating point rounding issue by not back-calculating from rate_cdf and just uding the direct rates to figure out fraction traveling.
…ing point error for cohort migration)
- Changed the code that read the migration metadata so that it uses the standard JsonConfigurable code. - Should not change any behavior except maybe error messages Co-authored-by: Svetlana Titova <[email protected]>
| virtual void ConfigInterpolationType( const Configuration* config ) override { /* don't need to check it */ }; | ||
| virtual void ConfigMigrationType( const Configuration* config, MigrationType::Enum& file_migration_type ) override { /* don't need to get it */ }; | ||
| virtual void CheckMigrationType( const Configuration* config, const MigrationType::Enum file_migration_type ) override { /* don't need to check it */ }; | ||
| virtual void ConfigDatavalueCount( const Configuration* config ) override; |
There was a problem hiding this comment.
to be clear, you could put all of these parameters in two methods: one for configuring and one for checking. It doesn't have to be two methods for each parameter. The main idea is to use inheritance to change the configuration.
| MigrationType::Enum file_migration_type = MigrationType::NO_MIGRATION; // default because needs to default to something coherent | ||
| ConfigMigrationType( config, file_migration_type ); | ||
| ConfigInterpolationType( config ); | ||
| ConfigDatavalueCount( config ); |
There was a problem hiding this comment.
I say this below, but you could just have one method that configures these three parameters and one method that checks them.
| std::string offsets_str; | ||
| initConfigTypeMap( "NodeOffsets", &offsets_str, MigrationOffsetsData_NodeOffsets_DESC_TEXT ); | ||
| initConfigTypeMap( "Metadata", m_pMetadata, MigrationOffsetsData_Metadata_DESC_TEXT ); | ||
|
|
There was a problem hiding this comment.
Note that I created two classes:MigrationMetadata and MigrationOffsets because that is the way the JSON was. We could have had one class if we could move all of the parameters that are used to the same level as NodeOffsets. If MetaData was really just comments, then it would be easier on the code. Typically, our code treats a JSON dictionary as an object and the elements of the dictionary as attributes/member variables of the object.
| @@ -10,6 +10,7 @@ | |||
| #include "IdmString.h" | |||
| #include "RANDOM.h" | |||
| #include "INodeContext.h" | |||
| #include "config_params.rc" | |||
There was a problem hiding this comment.
The migration data isn't part of config and the descriptions don't really have a home. I put some demographics descriptions somewhere....
|
|
||
| std::streamoff offset = (indexGender * gender_size) + (indexAge * age_size) + m_Offsets.at( nodeID ); | ||
| return offset; | ||
| } |
There was a problem hiding this comment.
Don't do anything. Just thinking out loud. After watching our videos on coupling and cohesion, I'm wishing this were right next to our code that calculates the expected binary file size. Hmm. It probably shows that it would be better if MigrationMetadata and MigrationOffsets were one class.
| m_FileStream.read( reinterpret_cast<char*>(array_id), m_DestinationsPerNode * sizeof(uint32_t) ); | ||
| m_FileStream.read( reinterpret_cast<char*>(array_rt), m_DestinationsPerNode * sizeof(double) ); | ||
| m_FileStream.read( reinterpret_cast<char*>(array_id), destinations_per_node * sizeof(uint32_t) ); | ||
| m_FileStream.read( reinterpret_cast<char*>(array_rt), destinations_per_node * sizeof(double) ); |
There was a problem hiding this comment.
More thought exercise. Assume MigrationMetadata and MigrationOffsets is really one thing. Would it be more cohesive if the "destinations_per_node * sizeof(xxx)" where in this combined class? Hmm.
| @@ -15,296 +17,136 @@ namespace Kernel | |||
| #define MODIFIER_EQUATION_NAME "Vector_Migration_Modifier_Equation" | |||
| #define MAX_DESTINATIONS (100) // maximum number of destinations per node in migration file | |||
|
|
|||
|
|
|||
| #define MMV_AlleleCombinations_DESC_TEXT "An array of allele combinations (pairs of alleles) to use for mapping migration rates to in the migration binary file. Depends on GenderDataType=VECTOR_MIGRATION_BY_GENETICS." | |||
There was a problem hiding this comment.
did you want to put this in config_params.rc?
…evious migrationType checking
regression tests: https://jenkins.apps.idmod.org/job/EMOD_Builds/job/scons_linux_all_regression/27/
sfts all: https://jenkins.apps.idmod.org/job/EMOD_Builds/job/scons_linux_all_sfts/28/ <-- failing the HIV/SFT/ARTMortalityTable/ARTDuration test: it is marked as "failing" in the hiv_science.json, so I think it's not an actual issue.