Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time trend BoxCox transform and weight trending #500

Merged
merged 14 commits into from
Sep 12, 2024

Conversation

jamesturner246
Copy link
Contributor

@jamesturner246 jamesturner246 commented Aug 21, 2024

Resolves #499
Resolves #501
Resolves #477

The main changes here are:

  • Implement BoxCox transformation on time trend factors
  • Implement recursive get_expected in KevinHallModel, to compute expected weight from nutrients
  • Remove default arguments to RiskFactorAdjustableModel methods to pre-empt bugs
  • Added a static gender to value method to person.cpp

Happy to walk through over a chat.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/HealthGPS/static_linear_model.cpp Show resolved Hide resolved
src/HealthGPS/static_linear_model.cpp Outdated Show resolved Hide resolved
@jamesturner246 jamesturner246 changed the title Time trend BoxCox transform Time trend BoxCox transform and weight trending Aug 22, 2024
@jamesturner246 jamesturner246 marked this pull request as ready for review August 30, 2024 14:03
@jzhu20
Copy link
Contributor

jzhu20 commented Sep 9, 2024

It looks good to me.

Copy link
Contributor

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few small comments... I'd be interested to have a walkthrough though if you've got time!

auto expected_trend = std::make_unique<std::unordered_map<core::Identifier, double>>();
auto expected_trend_boxcox = std::make_unique<std::unordered_map<core::Identifier, double>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we talked about this on the last PR... but I'm still a bit confused as to why these need to be std::unique_ptrs. Even if they're being converted into std::shared_ptrs at some point can't you just std::move your std::vector (for example) into a std::shared_ptr<std::vector<T>>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside model parser, probably. I'll have a look.

weight -= 0.6256 * get_expected(context, sex, age, "Height"_id, std::nullopt, false);
weight += 0.4925 * age;
weight -= 16.6166 * Person::gender_to_value(sex);
return weight;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do these coefficients come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -47,7 +47,9 @@ double Person::get_risk_factor_value(const core::Identifier &key) const {
throw std::out_of_range("Risk factor not found: " + key.to_string());
}

float Person::gender_to_value() const {
float Person::gender_to_value() const { return gender_to_value(gender); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether these helpers should be member functions of the core::Gender enum class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout.

@jamesturner246
Copy link
Contributor Author

I've made a few small comments... I'd be interested to have a walkthrough though if you've got time!

Sure! Today is good.

@alexdewar
Copy link
Contributor

Sure! Today is good.

Sorry, only just saw this... Could do tomorrow?

Copy link
Contributor

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jamesturner246 jamesturner246 merged commit 225e272 into main Sep 12, 2024
5 checks passed
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 72 lines in your changes missing coverage. Please review.

Project coverage is 46.17%. Comparing base (25a516f) to head (0a8ca32).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
src/HealthGPS/kevin_hall_model.cpp 0.00% 29 Missing ⚠️
src/HealthGPS/static_linear_model.cpp 0.00% 26 Missing ⚠️
src/HealthGPS.Input/model_parser.cpp 0.00% 11 Missing ⚠️
...rc/HealthGPS/dynamic_hierarchical_linear_model.cpp 0.00% 2 Missing ⚠️
src/HealthGPS/person.cpp 0.00% 2 Missing ⚠️
src/HealthGPS/risk_factor_adjustable_model.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
- Coverage   46.42%   46.17%   -0.26%     
==========================================
  Files         161      161              
  Lines        7756     7798      +42     
  Branches     1045     1054       +9     
==========================================
  Hits         3601     3601              
- Misses       3969     4011      +42     
  Partials      186      186              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamesturner246 jamesturner246 deleted the expect_weight_adjust branch September 12, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply time trend to body weight Add boxcox params for time trends Add time trend in risk factors
3 participants