From b82c7115ffdc60ec0b517f768fe793b07cac5db3 Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Sat, 29 Jul 2023 01:15:23 +0100 Subject: [PATCH 1/7] Remove risk_factor 'proxy' name and entity_key() from code. Just use regular key(). --- src/HealthGPS.Console/configuration.cpp | 4 ++-- src/HealthGPS.Console/jsonparser.cpp | 3 +-- src/HealthGPS.Console/poco.h | 1 - src/HealthGPS.Tests/HealthGPS.Test.cpp | 7 +++--- .../HierarchicalMapping.Test.cpp | 9 +------- src/HealthGPS/healthgps.cpp | 2 +- src/HealthGPS/hierarchical_model_static.cpp | 3 +-- src/HealthGPS/mapping.cpp | 18 ++------------- src/HealthGPS/mapping.h | 23 +------------------ 9 files changed, 12 insertions(+), 58 deletions(-) diff --git a/src/HealthGPS.Console/configuration.cpp b/src/HealthGPS.Console/configuration.cpp index fdf1228d5..ce90ded79 100644 --- a/src/HealthGPS.Console/configuration.cpp +++ b/src/HealthGPS.Console/configuration.cpp @@ -336,10 +336,10 @@ ModelInput create_model_input(core::DataTable &input_table, core::Country countr auto mapping = std::vector(); for (auto &item : config.modelling.risk_factors) { if (item.range.empty()) { - mapping.emplace_back(item.name, item.level, item.proxy); + mapping.emplace_back(item.name, item.level); } else { auto boundary = FactorRange{item.range[0], item.range[1]}; - mapping.emplace_back(item.name, item.level, item.proxy, boundary); + mapping.emplace_back(item.name, item.level, boundary); } } diff --git a/src/HealthGPS.Console/jsonparser.cpp b/src/HealthGPS.Console/jsonparser.cpp index f9f6a0f9d..771e6d7a9 100644 --- a/src/HealthGPS.Console/jsonparser.cpp +++ b/src/HealthGPS.Console/jsonparser.cpp @@ -115,13 +115,12 @@ void from_json(const json &j, BaselineInfo &p) { // Risk Factor Modelling void to_json(json &j, const RiskFactorInfo &p) { - j = json{{"name", p.name}, {"level", p.level}, {"proxy", p.proxy}, {"range", p.range}}; + j = json{{"name", p.name}, {"level", p.level}, {"range", p.range}}; } void from_json(const json &j, RiskFactorInfo &p) { j.at("name").get_to(p.name); j.at("level").get_to(p.level); - j.at("proxy").get_to(p.proxy); j.at("range").get_to(p.range); } diff --git a/src/HealthGPS.Console/poco.h b/src/HealthGPS.Console/poco.h index d4fe61605..75db4d288 100644 --- a/src/HealthGPS.Console/poco.h +++ b/src/HealthGPS.Console/poco.h @@ -35,7 +35,6 @@ struct BaselineInfo { struct RiskFactorInfo { std::string name; int level{}; - std::string proxy; std::vector range; }; diff --git a/src/HealthGPS.Tests/HealthGPS.Test.cpp b/src/HealthGPS.Tests/HealthGPS.Test.cpp index 24a47507c..ed3466630 100644 --- a/src/HealthGPS.Tests/HealthGPS.Test.cpp +++ b/src/HealthGPS.Tests/HealthGPS.Test.cpp @@ -17,8 +17,7 @@ namespace fs = std::filesystem; static std::vector create_mapping_entries() { using namespace hgps; - return {MappingEntry("Gender", 0, core::Identifier{"gender"}), - MappingEntry("Age", 0, core::Identifier{"age"}), MappingEntry("SmokingStatus", 1), + return {MappingEntry("Gender", 0), MappingEntry("Age", 0), MappingEntry("SmokingStatus", 1), MappingEntry("AlcoholConsumption", 1), MappingEntry("BMI", 2)}; } @@ -293,8 +292,8 @@ TEST(TestHealthGPS, ModuleFactoryRegistry) { auto mapping = HierarchicalMapping(std::vector{ MappingEntry("Year", 0), - MappingEntry("Gender", 0, core::Identifier{"gender"}), - MappingEntry("Age", 0, core::Identifier{"age"}), + MappingEntry("Gender", 0), + MappingEntry("Age", 0), }); auto diseases = std::vector{DiseaseInfo{.group = DiseaseGroup::other, diff --git a/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp b/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp index 108aabb3c..1aee3be9a 100644 --- a/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp +++ b/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp @@ -4,8 +4,7 @@ static std::vector create_mapping_entries() { using namespace hgps; - return {MappingEntry("Gender", 0, core::Identifier{"gender"}), - MappingEntry("Age", 0, core::Identifier{"age"}), MappingEntry("SmokingStatus", 1), + return {MappingEntry("Gender", 0), MappingEntry("Age", 0), MappingEntry("SmokingStatus", 1), MappingEntry("AlcoholConsumption", 1), MappingEntry("BMI", 2)}; } @@ -48,9 +47,6 @@ TEST(TestHealthGPS_Mapping, AccessByInterator) { ASSERT_EQ(exp_size, mapping.size()); for (auto &entry : mapping) { ASSERT_GE(entry.level(), 0); - if (!entry.is_entity()) { - EXPECT_EQ(entry.key(), entry.entity_key()); - } } } @@ -64,9 +60,6 @@ TEST(TestHealthGPS_Mapping, AccessByConstInterator) { ASSERT_EQ(exp_size, mapping.size()); for (const auto &entry : mapping) { ASSERT_GE(entry.level(), 0); - if (!entry.is_entity()) { - EXPECT_EQ(entry.key(), entry.entity_key()); - } } } diff --git a/src/HealthGPS/healthgps.cpp b/src/HealthGPS/healthgps.cpp index bda4b04d0..7a50a84e2 100644 --- a/src/HealthGPS/healthgps.cpp +++ b/src/HealthGPS/healthgps.cpp @@ -360,7 +360,7 @@ void hgps::HealthGPS::print_initial_population_statistics() { for (const auto &entity : context_.population()) { for (const auto &entry : context_.mapping()) { - sim8_summary[entry.name()].append(entity.get_risk_factor_value(entry.entity_key())); + sim8_summary[entry.name()].append(entity.get_risk_factor_value(entry.key())); } } diff --git a/src/HealthGPS/hierarchical_model_static.cpp b/src/HealthGPS/hierarchical_model_static.cpp index b8a247a06..3d5af6836 100644 --- a/src/HealthGPS/hierarchical_model_static.cpp +++ b/src/HealthGPS/hierarchical_model_static.cpp @@ -97,8 +97,7 @@ void StaticHierarchicalLinearModel::generate_for_entity(RuntimeContext &context, determ_risk_factors.emplace(InterceptKey, entity.get_risk_factor_value(InterceptKey)); for (const auto &item : context.mapping()) { if (item.level() < level) { - determ_risk_factors.emplace(item.key(), - entity.get_risk_factor_value(item.entity_key())); + determ_risk_factors.emplace(item.key(), entity.get_risk_factor_value(item.key())); } } diff --git a/src/HealthGPS/mapping.cpp b/src/HealthGPS/mapping.cpp index 82ea3bc6b..c65d7e3ac 100644 --- a/src/HealthGPS/mapping.cpp +++ b/src/HealthGPS/mapping.cpp @@ -8,27 +8,13 @@ namespace hgps { -MappingEntry::MappingEntry(std::string name, int level, core::Identifier entity_key, - FactorRange range) - : name_{name}, name_key_{core::Identifier{name}}, level_{level}, - entity_key_{std::move(entity_key)}, range_{range} {} - -MappingEntry::MappingEntry(std::string name, int level, core::Identifier entity_key) - : MappingEntry(name, level, entity_key, FactorRange{}) {} - -MappingEntry::MappingEntry(std::string name, int level) - : MappingEntry(name, level, core::Identifier::empty(), FactorRange{}) {} +MappingEntry::MappingEntry(std::string name, int level, FactorRange range) + : name_{name}, name_key_{core::Identifier{name}}, level_{level}, range_{range} {} const std::string &MappingEntry::name() const noexcept { return name_; } int MappingEntry::level() const noexcept { return level_; } -const core::Identifier &MappingEntry::entity_key() const noexcept { - return is_entity() ? entity_key_ : name_key_; -} - -bool MappingEntry::is_entity() const noexcept { return !entity_key_.is_empty(); } - const core::Identifier &MappingEntry::key() const noexcept { return name_key_; } const FactorRange &MappingEntry::range() const noexcept { return range_; } diff --git a/src/HealthGPS/mapping.h b/src/HealthGPS/mapping.h index 31f8ce7e9..b8877536b 100644 --- a/src/HealthGPS/mapping.h +++ b/src/HealthGPS/mapping.h @@ -51,20 +51,8 @@ class MappingEntry { /// @brief Initialises a new instance of the MappingEntry class /// @param name Risk factor name /// @param level The hierarchical level - /// @param entity_key The associated Person property, if exists /// @param range The factor range - MappingEntry(std::string name, int level, core::Identifier entity_key, FactorRange range); - - /// @brief Initialises a new instance of the MappingEntry class - /// @param name Risk factor name - /// @param level The hierarchical level - /// @param entity_key The associated Person property, if exists - MappingEntry(std::string name, int level, core::Identifier entity_key); - - /// @brief Initialises a new instance of the MappingEntry class - /// @param name Risk factor name - /// @param level The hierarchical level - MappingEntry(std::string name, int level); + MappingEntry(std::string name, int level, FactorRange range = {}); /// @brief Gets the factor name /// @return Factor name @@ -78,14 +66,6 @@ class MappingEntry { /// @return Factor identification const core::Identifier &key() const noexcept; - /// @brief Gets the factor's associated Person property identifier - /// @return Associated entry identifier - const core::Identifier &entity_key() const noexcept; - - /// @brief Determine whether this factor has an associated Person property, e.g. age - /// @return true, if the factor has an associated property; otherwise, false. - bool is_entity() const noexcept; - /// @brief Gets the factor allowed values range /// @return Factor values range const FactorRange &range() const noexcept; @@ -99,7 +79,6 @@ class MappingEntry { std::string name_; core::Identifier name_key_; int level_{}; - core::Identifier entity_key_; FactorRange range_; }; From 976294b09270a35fe4122a81cd9a4b5637edfb7f Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Sat, 29 Jul 2023 01:15:38 +0100 Subject: [PATCH 2/7] Remove risk_factor 'proxy' name from config JSONs. --- example/France.Config.json | 26 +++++++++++++++----------- example/France.EBHLM.json | 11 ----------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/example/France.Config.json b/example/France.Config.json index 548c2fcbd..03177f1a9 100644 --- a/example/France.Config.json +++ b/example/France.Config.json @@ -36,52 +36,56 @@ { "name": "Gender", "level": 0, - "proxy": "gender", "range": [0, 1] }, - { "name": "Age", "level": 0, "proxy": "age", "range": [1, 87] }, - { "name": "Age2", "level": 0, "proxy": "", "range": [1, 7569] }, - { "name": "Age3", "level": 0, "proxy": "", "range": [1, 658503] }, + { + "name": "Age", + "level": 0, + "range": [1, 87] + }, + { + "name": "Age2", + "level": 0, + "range": [1, 7569] + }, + { + "name": "Age3", + "level": 0, + "range": [1, 658503] + }, { "name": "SES", "level": 0, - "proxy": "ses", "range": [-2.316299, 2.296689] }, { "name": "Sodium", "level": 1, - "proxy": "", "range": [1.127384, 8.656519] }, { "name": "Protein", "level": 1, - "proxy": "", "range": [43.50682, 238.4145] }, { "name": "Fat", "level": 1, - "proxy": "", "range": [45.04756, 382.664098658922] }, { "name": "PA", "level": 2, - "proxy": "", "range": [22.22314, 9765.512] }, { "name": "Energy", "level": 2, - "proxy": "", "range": [1326.14051277588, 7522.496] }, { "name": "BMI", "level": 3, - "proxy": "", "range": [13.88, 39.48983] } ], diff --git a/example/France.EBHLM.json b/example/France.EBHLM.json index ac043cd1d..02e7919b4 100644 --- a/example/France.EBHLM.json +++ b/example/France.EBHLM.json @@ -11,67 +11,56 @@ { "Name": "Gender", "Level": 0, - "Proxy": "gender", "Range": [0, 1] }, { "Name": "Age", "Level": 0, - "Proxy": "age", "Range": [1, 87] }, { "Name": "Age2", "Level": 0, - "Proxy": "", "Range": [1, 7569] }, { "Name": "Age3", "Level": 0, - "Proxy": "", "Range": [1, 658503] }, { "Name": "SES", "Level": 0, - "Proxy": "ses", "Range": [-2.316299, 2.296689] }, { "Name": "Sodium", "Level": 1, - "Proxy": "", "Range": [1.127384, 8.656519] }, { "Name": "Protein", "Level": 1, - "Proxy": "", "Range": [43.50682, 238.4145] }, { "Name": "Fat", "Level": 1, - "Proxy": "", "Range": [45.04756, 382.664098658922] }, { "Name": "PA", "Level": 2, - "Proxy": "", "Range": [22.22314, 9765.512] }, { "Name": "Energy", "Level": 2, - "Proxy": "", "Range": [1326.14051277588, 7522.496] }, { "Name": "BMI", "Level": 3, - "Proxy": "", "Range": [13.88, 39.48983] } ], From f9568c0d58d1e41c99d2f821ca1121bca57e9266 Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Sat, 29 Jul 2023 01:16:10 +0100 Subject: [PATCH 3/7] Ignore local Python .venv. --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index dfc20c0de..a6673d849 100644 --- a/.gitignore +++ b/.gitignore @@ -415,3 +415,6 @@ FodyWeavers.xsd # Compile commands for clangd language server compile_commands.json + +# Python venv +.venv From 1994fb241b9e4ec8a9e2723cf2130fffe2090474 Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Mon, 31 Jul 2023 14:40:19 +0100 Subject: [PATCH 4/7] Tests: use brace initialisation where we can. --- src/HealthGPS.Tests/HealthGPS.Test.cpp | 10 ++-------- src/HealthGPS.Tests/HierarchicalMapping.Test.cpp | 4 +--- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/HealthGPS.Tests/HealthGPS.Test.cpp b/src/HealthGPS.Tests/HealthGPS.Test.cpp index ed3466630..1e73b6721 100644 --- a/src/HealthGPS.Tests/HealthGPS.Test.cpp +++ b/src/HealthGPS.Tests/HealthGPS.Test.cpp @@ -16,9 +16,7 @@ namespace fs = std::filesystem; static std::vector create_mapping_entries() { - using namespace hgps; - return {MappingEntry("Gender", 0), MappingEntry("Age", 0), MappingEntry("SmokingStatus", 1), - MappingEntry("AlcoholConsumption", 1), MappingEntry("BMI", 2)}; + return {{"Gender", 0}, {"Age", 0}, {"SmokingStatus", 1}, {"AlcoholConsumption", 1}, {"BMI", 2}}; } void create_test_datatable(hgps::core::DataTable &data) { @@ -290,11 +288,7 @@ TEST(TestHealthGPS, ModuleFactoryRegistry) { auto ses_mapping = std::map{{"test", builder.name()}}; auto ses = SESDefinition{.fuction_name = "normal", .parameters = std::vector{0.0, 1.0}}; - auto mapping = HierarchicalMapping(std::vector{ - MappingEntry("Year", 0), - MappingEntry("Gender", 0), - MappingEntry("Age", 0), - }); + auto mapping = HierarchicalMapping({{"Year", 0}, {"Gender", 0}, {"Age", 0}}); auto diseases = std::vector{DiseaseInfo{.group = DiseaseGroup::other, .code = core::Identifier{"angina"}, diff --git a/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp b/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp index 1aee3be9a..0c71ae04b 100644 --- a/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp +++ b/src/HealthGPS.Tests/HierarchicalMapping.Test.cpp @@ -3,9 +3,7 @@ #include "HealthGPS/mapping.h" static std::vector create_mapping_entries() { - using namespace hgps; - return {MappingEntry("Gender", 0), MappingEntry("Age", 0), MappingEntry("SmokingStatus", 1), - MappingEntry("AlcoholConsumption", 1), MappingEntry("BMI", 2)}; + return {{"Gender", 0}, {"Age", 0}, {"SmokingStatus", 1}, {"AlcoholConsumption", 1}, {"BMI", 2}}; } TEST(TestHealthGPS_Mapping, CreateEmpty) { From 16dcd3fc381a5437641fad40baf0c7f84549393f Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Mon, 31 Jul 2023 14:41:04 +0100 Subject: [PATCH 5/7] Don't copy twice. --- src/HealthGPS/mapping.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HealthGPS/mapping.cpp b/src/HealthGPS/mapping.cpp index c65d7e3ac..faf004a8f 100644 --- a/src/HealthGPS/mapping.cpp +++ b/src/HealthGPS/mapping.cpp @@ -9,7 +9,7 @@ namespace hgps { MappingEntry::MappingEntry(std::string name, int level, FactorRange range) - : name_{name}, name_key_{core::Identifier{name}}, level_{level}, range_{range} {} + : name_{std::move(name)}, name_key_{name_}, level_{level}, range_{range} {} const std::string &MappingEntry::name() const noexcept { return name_; } From 1c3a59e1610e15ea9bbc50ee501f9fc88b8352ca Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Mon, 31 Jul 2023 15:57:56 +0100 Subject: [PATCH 6/7] Replace FactorRange with built-in optional>. --- src/HealthGPS.Console/configuration.cpp | 2 +- src/HealthGPS/mapping.cpp | 15 +++++----- src/HealthGPS/mapping.h | 38 ++++--------------------- 3 files changed, 14 insertions(+), 41 deletions(-) diff --git a/src/HealthGPS.Console/configuration.cpp b/src/HealthGPS.Console/configuration.cpp index ce90ded79..85f30a841 100644 --- a/src/HealthGPS.Console/configuration.cpp +++ b/src/HealthGPS.Console/configuration.cpp @@ -338,7 +338,7 @@ ModelInput create_model_input(core::DataTable &input_table, core::Country countr if (item.range.empty()) { mapping.emplace_back(item.name, item.level); } else { - auto boundary = FactorRange{item.range[0], item.range[1]}; + auto boundary = hgps::OptionalRange{{item.range[0], item.range[1]}}; mapping.emplace_back(item.name, item.level, boundary); } } diff --git a/src/HealthGPS/mapping.cpp b/src/HealthGPS/mapping.cpp index faf004a8f..171404782 100644 --- a/src/HealthGPS/mapping.cpp +++ b/src/HealthGPS/mapping.cpp @@ -8,8 +8,8 @@ namespace hgps { -MappingEntry::MappingEntry(std::string name, int level, FactorRange range) - : name_{std::move(name)}, name_key_{name_}, level_{level}, range_{range} {} +MappingEntry::MappingEntry(std::string name, int level, OptionalRange range) + : name_{std::move(name)}, name_key_{name_}, level_{level}, range_{std::move(range)} {} const std::string &MappingEntry::name() const noexcept { return name_; } @@ -17,14 +17,13 @@ int MappingEntry::level() const noexcept { return level_; } const core::Identifier &MappingEntry::key() const noexcept { return name_key_; } -const FactorRange &MappingEntry::range() const noexcept { return range_; } +const OptionalRange &MappingEntry::range() const noexcept { return range_; } double MappingEntry::get_bounded_value(const double &value) const noexcept { - if (range_.empty) { - return value; + if (range_.has_value()) { + return std::min(std::max(value, range_->first), range_->second); } - - return std::min(std::max(value, range_.minimum), range_.maximum); + return value; } inline bool operator>(const MappingEntry &lhs, const MappingEntry &rhs) { @@ -74,4 +73,4 @@ std::vector HierarchicalMapping::at_level(int level) const noexcep return result; } -} // namespace hgps \ No newline at end of file +} // namespace hgps diff --git a/src/HealthGPS/mapping.h b/src/HealthGPS/mapping.h index b8877536b..8ee51ff76 100644 --- a/src/HealthGPS/mapping.h +++ b/src/HealthGPS/mapping.h @@ -11,34 +11,8 @@ namespace hgps { /// @brief The constant in the regression model presentation identifier inline const core::Identifier InterceptKey = core::Identifier{"intercept"}; -/// @brief Defines the risk factor allowed range data type -/// -/// @details The factors range is defined from the fitted dataset and enforced -/// by the simulation algorithm. The default constructor, creates an empty range. -struct FactorRange { - /// @brief Initialises a new instance of the FactorRange structure - FactorRange() = default; - - /// @brief Initialises a new instance of the FactorRange structure - /// @param min_value Minimum factor value - /// @param max_value Maximum factor value - /// @throws std::invalid_argument for minimum greater than the maximum value - FactorRange(double min_value, double max_value) - : empty{false}, minimum{min_value}, maximum{max_value} { - if (min_value > max_value) { - throw std::invalid_argument("Factor range minimum must not be greater than maximum."); - } - } - - /// @brief Gets a value indicating whether the range is empty, no limits. - bool empty{true}; - - /// @brief The range minimum value - double minimum{}; - - /// @brief The range maximum value - double maximum{}; -}; +/// @brief Optional Range of doubles data type +using OptionalRange = std::optional>; /// @brief Defines risk factor mapping entry data type /// @@ -52,7 +26,7 @@ class MappingEntry { /// @param name Risk factor name /// @param level The hierarchical level /// @param range The factor range - MappingEntry(std::string name, int level, FactorRange range = {}); + MappingEntry(std::string name, int level, OptionalRange range = {}); /// @brief Gets the factor name /// @return Factor name @@ -68,7 +42,7 @@ class MappingEntry { /// @brief Gets the factor allowed values range /// @return Factor values range - const FactorRange &range() const noexcept; + const OptionalRange &range() const noexcept; /// @brief Adjusts a value to the factor range, if provided /// @param value The value to adjust @@ -79,7 +53,7 @@ class MappingEntry { std::string name_; core::Identifier name_key_; int level_{}; - FactorRange range_; + OptionalRange range_; }; /// @brief Defines the hierarchical model mapping data type @@ -147,4 +121,4 @@ class HierarchicalMapping { std::vector mapping_; }; -} // namespace hgps \ No newline at end of file +} // namespace hgps From 011d59963a638566059e3caa00638312fd773b19 Mon Sep 17 00:00:00 2001 From: James Paul Turner Date: Mon, 31 Jul 2023 16:29:04 +0100 Subject: [PATCH 7/7] Replace FactorRange with built-in optional of pair of double,double.