Time trend for expected values #491
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@jamesturner246 I'm guessing you meant to tag us as reviewers rather than assigning this to us? |
|
Oh whoops. Thanks! |
| MEASURE_FUNCTION(); | ||
|
|
||
| const auto &info = config.modelling.baseline_adjustment; | ||
| if (!hgps::core::case_insensitive::equals(info.format, "CSV")) { | ||
| throw hgps::core::HgpsException{"Unsupported file format: " + info.format}; | ||
| } | ||
|
|
||
| auto table = hgps::RiskFactorSexAgeTable{}; | ||
| auto table = std::make_unique<hgps::RiskFactorSexAgeTable>(); |
There was a problem hiding this comment.
Why did you change table to a std::unique_ptr btw? Not a problem; just curious.
There was a problem hiding this comment.
I noted clang-tidy began complaining about model members as const and reference complicating automatic ctor generation -- see https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.htmlv and links therein.
I suppose I could have made it into a shared pointer within the model class, but thought this way we only std move a pointer to the definition ctor. 🤷
There was a problem hiding this comment.
Ah ok. That's interesting.
I think it's the right decision to return it as a std::unique_ptr. Out of interest, is it actually shared though? Or could it be left as a std::unique_ptr?
There was a problem hiding this comment.
It's shared once it enters the risk factor model definition class (with the risk factor model class), since it's technically a factory. I keep it unique here and move it into a shared pointer inside the definition class. Not sure if that's good practice or not..
| double expected = expected_->at(sex, factor).at(age); | ||
| if (expected_trend_->contains(factor)) { | ||
| int elapsed_time = context.time_now() - context.start_time(); | ||
| expected *= pow(expected_trend_->at(factor), elapsed_time); |
There was a problem hiding this comment.
Are these arguments the wrong way round? I'm trying to wrap my head round why you'd raise to the power of elapsed_time.
There was a problem hiding this comment.
Yeah, it' look a bit strange, but it's basically an exponential trend in time, With time factor t, we have with_trend = without_trend * t^elapsed. So for year zero, we have with_trend = without_trend * t^0 = without_trend, and t factor changes exponentially with years.
There was a problem hiding this comment.
That reminds me, I need to bound these values...
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
Resolves #492
This change adds new
StaticLinear.jsonparams:ExpectedTrend, which are raised to the power of the iteration number and used to multiply risk factor expected values, to simulate (exponentially) changing population average of nutrients over time.These are passed into adjustable models as shared pointers rather than references -- see https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.html
All adjustable RF model code is updated to use the new trends.