Not importing incidence 1?

[zkamvar]

at I'm mostly just curious, but what is the reasoning behind the design decision to copy over the code from {incidence} initially instead of importing? From my perspective, if there's a bug in the future, then there are two places where it needs to be fixed.

[TimTaylor]

That's a good question. Initially I thought that this may become {incidence} and it just evolved from there. At the start I didn't know how many functions would remain untouched so it made sense to start of with {incidence} and hack from there. Now it's near completion though I'm in two minds, so would be great to get your opinion:

My main issue with having it as an import is from the users perspective; having both {incidence} and {incidence2} installed makes it quite easy to load the wrong package. Think of a new user who installs {incidence2} but calls library(incidence). They may not even realise that they have the two packages installed and would be wondering why it doesn't work. I know there are two packages, but I have still done this many times over the last few weeks :-(

On the flip side your point about bugs is very important. I need to check which functions remain unchanged: From memory make_breaks_easier and possibly the get_interval functions.

Equally this relates to the question as to whether we should have both {incidence} and {incidence2} or whether it would be better to update {incidence}.

Thoughts??

[zkamvar]

While it would be nice to merge the packages, this would increase the dependency stack for packages like {EpiEstim} and {projections}.

As added context, there are epis who take issue with the fact that this package is even called incidence in the first place when it has no measure to indicate counts over time over population (reconhub/incidence#102).

I would like to change the name for {incidence}, but it's been published and there are a couple of packages that depend on it, so that's not much of an option unfortunately.

What I think we could do is we could create a "core" package that both {incidence} and {incidence2} import. This way, we can share common functions and reduce our maintenance efforts

Ultimately, there is still the issue with users mis-typing {incidence} instead of {incidence2} and I think it would be best to change the name of this package no matter what path we pick. IIRC, {incidence2} was picked in a similar vein as {ggplot2} vs {ggplot}, but the latter was never used widely and ultimately removed from CRAN.

It would have been nice to name this package {epicurve}, but that ship sailed in 2017. There's also no reason why we couldn't go with the French {epicourbe} or even absurd {epidance}.

[thibautjombart]

My preferred option would be to get back in time and deposit epicurve as an incubator package on CRAN back in 2015, probably alongside a string of other epi*** packages. But since I can't find the keys to my DeLorean, I'm left with suboptimal options.

The plan would be for incidence and incidence2 to live side by side for a transition phase, and for incidence to be retired/deprecated once we are confident all features are available in other packages. Not sure what the timeline will be, but to allow plenty of time for people to migrate to the new packages, this transition phase could last a couple of years. So there is indeed potential for duplication of efforts in duplicated code.

How big is the code overlap at this stage, and what are the concerned functions?

[TimTaylor]

That's annoying about epicurve! Coming from a mathematical background the name 'incidence' makes sense but understand the concerns of others. I think we will probably have to stick with the current name of incidence2.

The code overlaps somewhat but with the current implementations there is little that can be shared; only one or two functions at most so not worth the additional effort. The functionality is implemented around the underlying incidence class which is fundamentally different in both (one a vector with additional attributes and the other a subclass of tibble).

I agree with Thibaut that a transition phase is probably the best bet. I've already made a branch in projections to see how easy it is to port over, with no real issues. I'm not too worried about the dependencies for an interactive data exploration package such as this. The majority of users are likely to have dplyr installed and with the recent stable, 1.0.0, release they're being a lot more conservative with breaking changes.

[zkamvar]

I will say that I'm happy to transfer over maintainership of incidence to one of you two since I'm no longer terribly invested in working in epidemiology (aside from helping out with R4EPIs).

[zkamvar]

Also, if the plan is just to supersede incidence altogether (which I support), then it may be better to have this code in the main incidence repo in one of two ways:

1. as the main branch while keeping the old version in a separate "stable" branch for occasional updates to CRAN (if updates are needed before the release of version 2)
2. as a separate version 2 branch and the main branch keep up-to-date with CRAN

Whatever you decide to do, just make sure you don't get yourselves into an adegenet 2 kind of situation 😛

[TimTaylor]

Cheers Zhian. I'm happy to takeover maintaining incidence. Assuming @thibautjombart gives the thumbs up, would you be able to email CRAN ([email protected]), cc myself (email in profile) to give them a heads up. I see that you recently got a patch release out so unlikely (crosses fingers) to be any updates in the near future! 😄

[zkamvar]

Sounds good to me. The only thing I ask is that you keep the test coverage at or above the 98% threshold.

[zkamvar]

Also, @tjtnew, would you mind opening an issue on incidence adding your name as maintainer?