Read weight quantiles #252

dalonsoa · 2023-11-09T12:29:03Z

Implements the logic to read the CSV files with the weight quantiles. The relevant files are added and the StaitcLinear.json file is modified to actually use that data. It also updates the static linear model and its definition to actually handle this information, removing the placeholders used before.

Close #251

# Conflicts: # src/HealthGPS/static_linear_model.cpp

github-actions

clang-tidy made some suggestions

src/HealthGPS/static_linear_model.cpp

src/HealthGPS/static_linear_model.h

github-actions · 2023-11-09T12:54:21Z

clang-tidy review says "All clean, LGTM! 👍"

alexdewar

I found one small mistake, but otherwise it's all good except for a few queries. I think we need to rework the DataTypeTableColumn API a bit, because it's not v nice to use atm.

example_new/new_data/weight_quantiles_NCDRisk_female.csv

alexdewar · 2023-11-09T13:52:06Z

src/HealthGPS.Console/model_parser.cpp

+    std::map<hgps::core::Gender, std::vector<double>> weight_quantiles = {
+        {hgps::core::Gender::female, std::vector<double>(quantiles_female.num_rows())},
+        {hgps::core::Gender::male, std::vector<double>(quantiles_female.num_columns())}};
+    for (size_t j = 0; j < quantiles_female.num_rows(); j++) {
+        weight_quantiles[hgps::core::Gender::female][j] =
+            std::any_cast<double>(quantiles_female.column(0).value(j));
+    }
+    for (size_t j = 0; j < quantiles_male.num_rows(); j++) {
+        weight_quantiles[hgps::core::Gender::male][j] =
+            std::any_cast<double>(quantiles_male.column(0).value(j));
+    }


I guess you intended the male vector to be quantiles_male.num_rows() in size?

That aside, it's better not to resize your vectors upfront, but to resize them as you go along. If you want to hint to the compiler that you know the final size you can use the reserve() method.

I think this should work:

Suggested change

std::map<hgps::core::Gender, std::vector<double>> weight_quantiles = {

{hgps::core::Gender::female, std::vector<double>(quantiles_female.num_rows())},

{hgps::core::Gender::male, std::vector<double>(quantiles_female.num_columns())}};

for (size_t j = 0; j < quantiles_female.num_rows(); j++) {

weight_quantiles[hgps::core::Gender::female][j] =

std::any_cast<double>(quantiles_female.column(0).value(j));

}

for (size_t j = 0; j < quantiles_male.num_rows(); j++) {

weight_quantiles[hgps::core::Gender::male][j] =

std::any_cast<double>(quantiles_male.column(0).value(j));

}

std::map<hgps::core::Gender, std::vector<double>> weight_quantiles = {

{hgps::core::Gender::female, {}},

{hgps::core::Gender::male, {}};

weight_quantiles[hgps::core::Gender::female].reserve(quantiles_female.num_rows());

weight_quantiles[hgps::core::Gender::male].reserve(quantiles_male.num_rows());

for (size_t j = 0; j < quantiles_female.num_rows(); j++) {

weight_quantiles[hgps::core::Gender::female].push_back(std::any_cast<double>(quantiles_female.column(0).value(j)));

}

for (size_t j = 0; j < quantiles_female.num_rows(); j++) {

weight_quantiles[hgps::core::Gender::male].push_back(std::any_cast<double>(quantiles_male.column(0).value(j)));

}

That said, this is rather ugly. I see this is where you wanted to use #253.

The problem is we get a DataTypeColumn here (the abstract base class), whereas your fix applies to PrimitiveDataTypeColumn -- I can't think of a better way to reorganise this API without digging into it a bit more though (which I'm happy to do, @jamesturner246!).

One option would be to use dynamic_cast to convert the DataTypeColumns into PrimitiveDataTypeColumn<double>s, then we can use your fix in #253 to get at the underlying std::vectors. What do you reckon, @jamesturner246?

PS -- The way you've done it is also fine (aside from the sizing problem) and if we're going to rework the API anyway, there's not much point in mucking about with it now if it works.

Yep, this is where I was thinking on using that patch.

I thought on using push_back, but for some reason I thought it was going to be less efficient that just allocating the space upfront, given that I knew it in advance, and just populating the elements.

Thanks for pointing the male/female issue. I'l fix it asap.

If not using something similar to #253, the only other option I can think of is implementing my own parser to read those files, but it is a pity given that there is already perfectly fine functionality to do that.

It's likely to be a really minor performance difference in this case, but the reason not to resize containers when creating them is because the contents then have to be default-initialised (in this case that just means they'll be zeroed). If you use reserve() instead, you still allocate memory but there's no initialisation required.

If not using something similar to #253, the only other option I can think of is implementing my own parser to read those files, but it is a pity given that there is already perfectly fine functionality to do that.

I think we could rework the API so that you could get the values out by doing something like:

std::vector<double> my_values = column.values<double>();

If column contains ints or something else, you would get an exception.

Keeping aside mentioning the specific type, this was my intention, but it seems I did it in the wrong place.

You would need to update PrimitiveDataColumnType as you've done as well, but we also need to figure out how to change the base class to make use of your new values() method. Python makes this so much easier 😆

I think this can wait until afterwards. Lots of changes going on there at the moment.

src/HealthGPS/static_linear_model.cpp

Co-authored-by: Alex Dewar <[email protected]>

github-actions · 2023-11-09T16:07:43Z

clang-tidy review says "All clean, LGTM! 👍"

…EPI/healthgps into read_weight_quantiles

github-actions · 2023-11-09T16:11:36Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-11-09T16:18:10Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-11-10T08:30:32Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-11-10T12:32:47Z

clang-tidy review says "All clean, LGTM! 👍"

dalonsoa · 2023-11-10T12:42:50Z

@jamesturner246 , this seems to work after the last change. So if we are moving forward with the merging, I'd suggest we first merge this PR and then #250 .

jamesturner246

Looks good. I'd just add the same vector checking to ModelDefinition ctor as well, and good to go.

jamesturner246 · 2023-11-10T14:04:01Z

src/HealthGPS/static_linear_model.cpp

-    //}
+    if (weight_quantiles_.empty()) {
+        throw core::HgpsException("Weight quantiles dictionary is empty");
+    }


I'd add the same check to *ModelDefinition below.

…EPI/healthgps into read_weight_quantiles

github-actions · 2023-11-10T14:50:06Z

clang-tidy review says "All clean, LGTM! 👍"

dalonsoa added 6 commits November 9, 2023 06:41

Add weight quantiles data

eca593b

Add WeightQuantilesFiles definition to StaticLinear.json

27e85f3

Merge branch 'initialise_weights' into read_weight_quantiles

d768f30

Rename weight quantiles input information

eb3e961

Add reading of quantile files

d10f7a9

Merge branch 'initialise_weights' into read_weight_quantiles

625eeff

# Conflicts: # src/HealthGPS/static_linear_model.cpp

dalonsoa requested review from alexdewar and jamesturner246 and removed request for alexdewar November 9, 2023 12:39

github-actions bot reviewed Nov 9, 2023

View reviewed changes

src/HealthGPS/static_linear_model.cpp Outdated Show resolved Hide resolved

src/HealthGPS/static_linear_model.cpp Show resolved Hide resolved

src/HealthGPS/static_linear_model.h Outdated Show resolved Hide resolved

Fix clang-tidy complains

13a6912

alexdewar reviewed Nov 9, 2023

View reviewed changes

dalonsoa and others added 2 commits November 9, 2023 16:00

Use generator and static cast properly

9e1543f

Update src/HealthGPS/static_linear_model.cpp

f76ad03

Co-authored-by: Alex Dewar <[email protected]>

dalonsoa added 2 commits November 9, 2023 16:11

Include review comments

cceb1c8

Merge branch 'read_weight_quantiles' of https://github.com/imperialCH…

a5f7317

…EPI/healthgps into read_weight_quantiles

Fix complain of compiler on int conversion

f587159

dalonsoa linked an issue Nov 10, 2023 that may be closed by this pull request

Read weight quantiles #251

Closed

dalonsoa and others added 2 commits November 10, 2023 12:23

Fix complain of compiler on int conversion, again

7c957c7

Merge branch 'initialise_weights' into read_weight_quantiles

07ed546

jamesturner246 approved these changes Nov 10, 2023

View reviewed changes

Add validity check to inputs of model definition

d694913

Merge branch 'read_weight_quantiles' of https://github.com/imperialCH…

8ccd41b

…EPI/healthgps into read_weight_quantiles

jamesturner246 merged commit d3d9a2b into initialise_weights Nov 10, 2023
4 of 5 checks passed

jamesturner246 deleted the read_weight_quantiles branch November 10, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read weight quantiles #252

Read weight quantiles #252

dalonsoa commented Nov 9, 2023

github-actions bot left a comment

github-actions bot commented Nov 9, 2023

alexdewar left a comment

alexdewar Nov 9, 2023

alexdewar Nov 9, 2023

dalonsoa Nov 9, 2023

dalonsoa Nov 9, 2023

alexdewar Nov 9, 2023

alexdewar Nov 9, 2023

dalonsoa Nov 9, 2023

alexdewar Nov 9, 2023

jamesturner246 Nov 10, 2023

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

dalonsoa commented Nov 10, 2023

jamesturner246 left a comment

jamesturner246 Nov 10, 2023

dalonsoa Nov 10, 2023

github-actions bot commented Nov 10, 2023

Read weight quantiles #252

Read weight quantiles #252

Conversation

dalonsoa commented Nov 9, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2023

alexdewar 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

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

dalonsoa commented Nov 10, 2023

jamesturner246 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 10, 2023