Added AlterMakeham, AlterGompertz, Carriere1 and Carriere2 models#120
Added AlterMakeham, AlterGompertz, Carriere1 and Carriere2 models#120leeyuntien wants to merge 2 commits intoJuliaActuary:masterfrom
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! |
|
|
||
|
|
||
| """ | ||
| AlterMakeham(;μ,σ,c) |
There was a problem hiding this comment.
I think Makeham2 would be more consistent with the rest of the names. Same with AlterGompertz
alecloudenback
left a comment
There was a problem hiding this comment.
Todo:
- Change the
AlterMakehamnames 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.