Load nutrients #199

jamesturner246 · 2023-08-08T16:19:24Z

Fixes #162

I'll get this going, as the bulk of it is there.

I have a new example directory example_new, containing the new configs. France.NewEBM.json has another field: FoodsDataFile, specifying the CSV foods nutrients file similar to existing syntax in main config file./

the data is loaded inside model_parser load_newebm_model, and for now that's it. I did a little tidy up in csvparser as well. The helper functions are put in an anonymous namespace in csvparser.cpp, and declarations are removed from the header. This should make the interfaces a little less cluttered with helper fns.

I'll finalise and merge this after #200 merges, since it uses a bit of common code, which is now changing.

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-08T16:27:16Z

src/HealthGPS.Console/csvparser.cpp

-            return hc::case_insensitive::equals(pair.first, str);
+    std::map<std::string, std::string, hc::case_insensitive::comparator> csv_column_map;
+    for (const auto &[col_name, col_type] : columns) {
+        auto is_match = [&col_name](const auto &csv_col_name) {


warning: 'col_name' in capture list does not name a variable [clang-diagnostic-error]

auto is_match = [&col_name](const auto &csv_col_name) { ^

This seems to be a bug in clang, where it can't capture structured bindings in lambdas, even though C++20 allows it. https://stackoverflow.com/questions/46114214/lambda-implicit-capture-fails-with-variable-declared-from-structured-binding

Work to fix it here: https://reviews.llvm.org/D122768

For now I guess we don't use structured bindings here, unless we can work around that lambda.

github-actions · 2023-08-08T16:27:16Z

src/HealthGPS.Console/csvparser.cpp

+    std::map<std::string, std::string, hc::case_insensitive::comparator> csv_column_map;
+    for (const auto &[col_name, col_type] : columns) {
+        auto is_match = [&col_name](const auto &csv_col_name) {
+            return hc::case_insensitive::equals(col_name, csv_col_name);


warning: reference to local binding 'col_name' declared in enclosing function 'host::load_datatable_from_csv' [clang-diagnostic-error]

return hc::case_insensitive::equals(col_name, csv_col_name); ^

Additional context

src/HealthGPS.Console/csvparser.cpp:111: 'col_name' declared here

for (const auto &[col_name, col_type] : columns) { ^

github-actions · 2023-08-09T12:39:23Z

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

github-actions · 2023-08-09T14:31:02Z

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

dalonsoa

It looks good. I've just made a comment on reusability of code blocks and another one on using Excel files.

example_new/MAFFCODE with NUTRIENTS and NPM scores.xlsx

dalonsoa · 2023-08-09T14:46:48Z

src/HealthGPS.Console/configuration.cpp

+        file_path = config.file.name;
+        if (file_path.is_relative()) {
+            file_path = config.root_path / file_path;
+            config.file.name = file_path.string();


This same structure to make the path absolute is used in several places. I would suggest to write a small function that does the job and that you can reuse.

I've done this on my other branch, so maybe hold fire @jamesturner246. I'm happy to fix this up for you

Will do, thanks.

dalonsoa · 2023-08-09T14:47:25Z

src/HealthGPS.Console/configuration.cpp

+            file_path = model.second;
+            if (file_path.is_relative()) {
+                file_path = config.root_path / file_path;
+                model.second = file_path.string();


dalonsoa · 2023-08-09T14:47:32Z

src/HealthGPS.Console/configuration.cpp

+            file_path = item.second;
+            if (file_path.is_relative()) {
+                file_path = config.root_path / file_path;
+                item.second = file_path.string();
+            }


src/HealthGPS.Console/csvparser.cpp

alexdewar

The code changes look sensible to me. I've made some minor suggestions.

I'm not entirely clear what the contents of the example_new folder are though, other than that they're intended to be ingested by the new model. How do the files differ from those in example/ if at all? Are they just going to be example data? Or were you intending to replace them with real-world data at some point? Real-world data should probably live somewhere outside this repo; example data's fine, but could we possibly use a smaller dataset? I know the existing example dataset is huge, but I don't know that we want to commit 100,000s of lines of data to the repo every time we add a new model. Maybe example_new needs a readme to explain things a bit?

I think we should probably merge this before #200, because a) it'll be ready sooner and b) the refactoring on #200 is more extensive, so I think it's easier to do it this way round. In future we probs need to coordinate a bit better because there are going to be a whole lot of merge conflicts here (I'm not saying this is your fault!).

alexdewar · 2023-08-09T14:56:51Z

example_new/MAFFCODE_NUTRIENTS_NPM.csv

+310103,"Cornsnacks, based on maize",31.9,11.77,12.91,5.78,59.3,537,1.13,4.6,0,0,2.1,0,7,21
+310104,Wheat based savoury snack,28,17.24,7.71,1.76,51.8,488,0.935,2.2,0,0,5.7,46,10.4,5
+310201,Popcorn,20,1.72,9.48,7.01,77.6,480,0.056,62.1,61.8,0,1.8,18,2.1,15
+310301,Other savoury snacks (including hors d'oeuvres),16.246,5.924,7.152,1.933,13.365,244.639,0.402103,0.287,0.012,0,0.53,57.59,12.12,0


I love that there's someone out there looking at the health impact of eating hors d'oeuvres 😆

I hope they put a lot of thought into it! xD

example_new/MAFFCODE with NUTRIENTS and NPM scores.xlsx

alexdewar · 2023-08-09T14:58:33Z

example_new/France.HLM.json

Is this a straight copy of the other France.HLM.json file?

Yes. Temporary until the new initialisation is worked out. The dynamic model name is different: NewEBM, but that's it.

alexdewar · 2023-08-09T15:01:09Z

example_new/MAFFCODE_NUTRIENTS_NPM.csv

I'm not entirely clear on what the difference between the current example and data folders is, but might this belong in a data folder instead? The other one has things like country codes in there.

It's all quite temporary for the moment, until data and the rest of the model comes in, and we can organise it properly.

A better explanation: I wasn't really sure about this myself at first.

The data dir contains more permanent and widely applicable data, such as country codes (don't change often), diseases (also don't), etcetera.

The example dir contains input data for the current experiment, such as population initialisation and extrapolation data for France, at year 20xx.

There's probably some improvements to be made here, but, for now at least, there's separation of generally common and static data, and the data for the current experiment (year, country, etcetera). Hope that makes more sense.

alexdewar · 2023-08-09T15:02:08Z

src/HealthGPS.Console/configuration.cpp

+        file_path = config.file.name;
+        if (file_path.is_relative()) {
+            file_path = config.root_path / file_path;
+            config.file.name = file_path.string();


I've done this on my other branch, so maybe hold fire @jamesturner246. I'm happy to fix this up for you

src/HealthGPS.Console/csvparser.cpp

jamesturner246 · 2023-08-09T15:48:40Z

The examples_new is quite temporary. It's using parts of the older model, until the new model and data come in. I made a new dir temporarily so I can change things more freely without breaking the old one. Given the state of flux, I don't think we should prematurely optimise these until we actually know what's going in them.

Agree on the coordination point .Understand it's a bit annoying. it's quite hard to change things at the moment, given how tightly coupled the various config, settings, options, poco things are.

github-actions · 2023-08-10T12:34:20Z

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

alexdewar

LGTM!

src/HealthGPS.Console/csvparser.cpp

github-actions · 2023-08-11T11:15:57Z

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

jamesturner246 added 3 commits August 8, 2023 16:16

Add nutrient data.

d705b72

CVS parser: Small tidy up.

d730445

CVS parser: throw exception on loading error.

baef5bd

github-actions bot reviewed Aug 8, 2023

View reviewed changes

jamesturner246 added 8 commits August 8, 2023 18:54

Add NewEBM config to example_new (copy, do it doesn't clobber 161.

1ede14c

Add nutrient csv file info to NewEBM config.

b971ecf

Tidy up configuration a bit, and store root config path.

ee6a58f

Sync with main and 161 a bit.

4b19722

Copy remaining example files to example_new for now.

902859d

Pass whole config to register_risk_factor_model_definitions.

5a606b2

Remove unused fn, and use anon namespace for helper fns.

4299d7d

Handle lambdas not capturing structured bindings.

1ccb8f3

jamesturner246 added 5 commits August 9, 2023 14:12

Model parser: pass around whole configy, rather than small bits of it.

cdef4b7

Add Columns to NewEBM FoodsDataFile.

3f531cb

Fix some bugs uin last commit.

6d94117

Load food nutrients file in newebm model parser.

daa7b83

load_datatable_from_csv should take a FileInfo object.

9cd5758

jamesturner246 requested review from dalonsoa, alexdewar and israel-vieira August 9, 2023 14:31

jamesturner246 marked this pull request as ready for review August 9, 2023 14:32

dalonsoa approved these changes Aug 9, 2023

View reviewed changes

jamesturner246 commented Aug 9, 2023

View reviewed changes

src/HealthGPS.Console/csvparser.cpp Outdated Show resolved Hide resolved

alexdewar requested changes Aug 9, 2023

View reviewed changes

jamesturner246 added 3 commits August 10, 2023 11:39

Remove excel file (contains same data as CSV file.

5974230

Wrap DataTable mutex in unique_ptr to allow move semantics.

e4cf099

Forgot to init unique mutex ptr.

bd9fa6c

load_datatable_from_csv should return a DataTable object.

84f4c1e

jamesturner246 requested a review from alexdewar August 10, 2023 12:28

alexdewar approved these changes Aug 11, 2023

View reviewed changes

src/HealthGPS.Console/csvparser.cpp Outdated Show resolved Hide resolved

Don't need default-init braces.

7aa9aa4

jamesturner246 enabled auto-merge August 11, 2023 11:09

jamesturner246 merged commit e40b141 into main Aug 11, 2023

jamesturner246 deleted the load_nutrients branch August 11, 2023 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load nutrients #199

Load nutrients #199

jamesturner246 commented Aug 8, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot Aug 8, 2023

jamesturner246 Aug 8, 2023

jamesturner246 Aug 8, 2023

github-actions bot Aug 8, 2023

jamesturner246 Aug 9, 2023

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

dalonsoa left a comment

dalonsoa Aug 9, 2023

alexdewar Aug 9, 2023

jamesturner246 Aug 9, 2023

dalonsoa Aug 9, 2023

dalonsoa Aug 9, 2023

alexdewar left a comment

alexdewar Aug 9, 2023

jamesturner246 Aug 10, 2023

alexdewar Aug 9, 2023

jamesturner246 Aug 9, 2023

alexdewar Aug 9, 2023

jamesturner246 Aug 9, 2023

jamesturner246 Aug 10, 2023

alexdewar Aug 9, 2023

jamesturner246 commented Aug 9, 2023

github-actions bot commented Aug 10, 2023

alexdewar left a comment

github-actions bot commented Aug 11, 2023

Load nutrients #199

Load nutrients #199

Conversation

jamesturner246 commented Aug 8, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

dalonsoa 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

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

jamesturner246 commented Aug 9, 2023

github-actions bot commented Aug 10, 2023

alexdewar left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023

jamesturner246 commented Aug 8, 2023 •

edited

Loading