reconhub / incidence

☣:chart_with_upwards_trend::chart_with_downwards_trend:☣ Compute and visualise incidence

https://reconhub.github.io/incidence

Other

58 stars 13 forks source link

Allow separate fits per group for `fit_optim_split()` #59

Closed zkamvar closed 6 years ago

zkamvar commented 6 years ago

This implements an option separate_split in fit_optim_split() that allows the groups to have separate split dates to fix #39. I've also raised the coverage to >97%

Changes

fit_optim_split() now gains a separate_split argument that will determine the optimal split separately for groups.
A new class, incidence_fit_list, has been implemented to store and summarise incidence_fit objects within a nested list. This is the class returned by in the $fit element of fit_optim_split().
find_peak() gains the argument pool, giving the user the choice to find the peak per group or pooled.
get_counts() returns the matrix of case counts for each date.
get_fit() returns a list of incidence_fit objects from an incidence_fit_list object.
get_info() returns information stored in the $info element of an incidence_fit/incidence_fit_list object.

zkamvar commented 6 years ago

If there are no objections, I will merge this PR tomorrow.

caijun commented 6 years ago

@thibautjombart is more familiar with the implementation of fit_optim_split() than me, perhaps requesting review from him is better.

zkamvar commented 6 years ago

True, but this fixes the issue you raised, so I want to make sure it works as you expect. Mainly what I'm looking for is for another set of eyes to look at the code and make sure there's nothing obviously wrong with it before I merge it.

On Aug 10, 2018, at 02:17 , Jun Cai notifications@github.com wrote:

@thibautjombart https://github.com/thibautjombart is more familiar with the implementation of fit_optim_split() than me, perhaps requesting review from him is better.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/reconhub/incidence/pull/59#issuecomment-411945953, or mute the thread https://github.com/notifications/unsubscribe-auth/ADeIljoBn3mC3JaGYRmJ2d284emjC60Mks5uPN8wgaJpZM4V1ch3.

caijun commented 6 years ago

I commented under https://github.com/reconhub/incidence/issues/39, it perfectly solved my issue and I like the way in which the returning objects are organized, e.g. best.fit2$fit$`San Andres`$before. The access is so convenient. 👍

zkamvar commented 6 years ago

I'll consider that good enough. And if @thibautjombart doesn't like this change, then he shouldn't have made me maintainer :stuck_out_tongue_winking_eye: