Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sector and income #230
Sector and income #230
Changes from all commits
de623fd
d638cef
585e936
1922906
3a6534e
e5a312c
87c4eac
5f9a7fe
eb35572
c404c0a
f4d6638
b4d72ab
d7f5b61
7c0846f
1aea169
3c6ad90
c72c2bc
61ab5b7
06e553e
e373e53
f5a22d2
2326ca9
3985efa
905223a
71926bd
d32fd44
a44f19c
0ab8ef6
1ebaa1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debatable as to whether this would be an improvement, but FYI you can
zip
ranges together in C++20, as you would with Python: https://en.cppreference.com/w/cpp/ranges/zip_viewIn this case you could zip
opt["RiskFactorModels"]
andcorrelations_table
(which is also a range because it hascbegin
andcend
methods) and then you avoid definingi
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a C++23 feature :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using floating-point numbers for currency is a common gotcha, because of rounding errors. Do you think it would work to use an integer type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I didn't do that as I didn't think much arithmetic would happen with them. They aren't used yet, and will presumably be just logged for financial impact of interventions. They can be changed to unsigned integral types for pence/cents/... I'll leave that for another time, if and as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this model not need to generate risk factors or is this just something you haven't implemented yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't generate risk factors. That's done in e.g.
StaticHierarchicalLinearModel
. Alas, the confusion of using the generate (static) model and update (dynamic) model terminology when both models have generate and update methods. That's why I will do #231.