Trend max steps #528
Trend max steps #528
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #528 +/- ##
==========================================
- Coverage 49.12% 49.00% -0.13%
==========================================
Files 164 164
Lines 7796 7816 +20
Branches 1062 1063 +1
==========================================
Hits 3830 3830
- Misses 3957 3977 +20
Partials 9 9 ☔ View full report in Codecov by Sentry. |
|
It looks good to me. |
| @@ -44,7 +46,8 @@ double RiskFactorAdjustableModel::get_expected(RuntimeContext &context, core::Ge | |||
| // Apply optional trend to expected value. | |||
| if (apply_trend) { | |||
| int elapsed_time = context.time_now() - context.start_time(); | |||
| expected *= pow(expected_trend_->at(factor), elapsed_time); | |||
| int t = std::min(elapsed_time, get_trend_steps(factor)); | |||
| expected *= pow(expected_trend_->at(factor), t); | |||
There was a problem hiding this comment.
Could you explain the logic of this bit? It's puzzling me.
There was a problem hiding this comment.
Sure. t is the smaller of the number of elapsed years and the upper bound on elapsed years, used to bound the exponential growth. Maybe num_trend_steps is a better name.
So at the start, when elapsed <= trend_steps, we have expect_new = expect * trend^elapsed, but after we have expect_new = expect * trend^trend_steps.
| std::unique_ptr<std::unordered_map<core::Identifier, double>> expected_trend, | ||
| std::unique_ptr<std::unordered_map<core::Identifier, int>> trend_steps) |
There was a problem hiding this comment.
Am I right in thinking that expected_trend and trend_steps will have the same keys? If so, would it make sense to just have the one map, with each value being a struct containing both of these values?
There was a problem hiding this comment.
Mind If I do this one later? Still not completely certain that a new bunch of vars won't be requested soon.
|
I've opened #544 regarding the schema update. Sorry, should have done it here. |
REVIEW AFTER #527
This change adds a new configurable
TrendStepsvalue (must be defined for both the static (i.e.StaticLinear.json) and the dynamic (i.e.KevinHall.json) models).This is used to ensure that the exponential trends in
RiskFactorAdjustableModelandStaticLinearModelare capped after a defined number of elapsed time steps.Resolves #517