-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added AlterMakeham, AlterGompertz, Carriere1 and Carriere2 models #120
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #120 +/- ##
==========================================
- Coverage 89.97% 80.77% -9.20%
==========================================
Files 9 9
Lines 369 411 +42
==========================================
Hits 332 332
- Misses 37 79 +42
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thank you! Can you also add tests? I may be a bit delayed in reviewing the PRs, thank you for your patience! |
src/parameterized_models.jl
Outdated
@@ -48,6 +48,44 @@ end | |||
survival(m::Makeham,age) = exp(-cumhazard(m,age)) | |||
|
|||
|
|||
""" | |||
AlterMakeham(;μ,σ,c) |
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.
I think Makeham2
would be more consistent with the rest of the names. Same with AlterGompertz
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.
Todo:
- Change the
AlterMakeham
names to aMakeham2
- Add tests to compare the answers to a different source
Hey @leeyuntien you commented on the associated issue. The reason I haven't merged this is to address the "to-dos" in my prior comment. |
i have changed some function names in the fork more-model, please review, thanks |
Thanks for making the name changes. Can you add some test cases? E.g. see some of the existing cases here: https://github.com/JuliaActuary/MortalityTables.jl/blob/master/test/parameterized_models.jl Before merging new things we try to ensure correctness by testing the output of the new features/code. If you need help with that I can do one of the functions you added as an example. |
This is in response to the issue #46 Parameterized Models to add 4 models, including AlterMakeham, AlterGompertz, Carriere1 and Carriere2.