Open jrcpulliam opened 5 years ago
Note that I'm concerned that this PR and #90 both introduce new error messages that might break backward compatibility.
Also, @thibautjombart: what do you think about squashing commits (eg the ones where I'm just adding spaces?)
Note that I'm concerned that this PR and #90 both introduce new error messages that might break backward compatibility.
Can you explain what you mean by breaking backward compatibility? What breaks with #90 and this PR? I'm aware of the breaking change for character data and I might have a less break-y way of addressing that.
what do you think about squashing commits (eg the ones where I'm just adding spaces?
I don't know if there is a way to squash specific commits, either we take them all or squish them into one commit to rule them all.
@jrcpulliam, Thank you for the contribution. Sorry it's taken me a bit to get to this.
I have a couple of questions/comments on this:
|x ----0------------ y| ... Inf // Before conversion
|y ... Inf| // After conversion
Is this the correct interpretation (i.e. that it's impossible to have a negative doubling/halving time so the upper limit becomes the lower limit)?
For the growth rate, in this situation the CI is an exclusion interval:
r <- seq(-3,3,le = 100)
plot(r, log(2)/r, type = "l")
(sorry reprex not working with emacs)
So we may want to output something along the lines of [-Inf ; lower_bound] U [upper_bound ; +inf]
. Well, that's an option, happy to go with something else. @jrcpulliam what do you think?
I think part of the confusing stuff is we try to be clever in the output and report positive values as 'doubling times' and negative ones as 'halving times'. We could be more consistent and just report 'doubling/halving times', and explain that negative values are halving times?
Re the error: I think it comes from the above, e.g. one group may have r>0 and therefore a 'doubling time', while another would have r<0 and halving times reported.
I think part of the confusing stuff is we try to be clever in the output and report positive values as 'doubling times' and negative ones as 'halving times'. We could be more consistent and just report 'doubling/halving times', and explain that negative values are halving times?
I agree that the confusion is due to the way we handle the output. Currently, we try to detect whether r is negative or positive and then use that to determine if we want to use log(2) / r
or log(0.5) / r
to calculate the estimated time.
Re the error: I think it comes from the above, e.g. one group may have r>0 and therefore a 'doubling time', while another would have r<0 and halving times reported.
This is in part because we based the conditional only on the first r value, which can be changed. The situation that would cause this condition is not that infeasible and we should be able to address it.
All in all, I think that there is still room for us to give a sensible interpretation of the doubling/halving times. The way I see it, we calculate these values to make the lives of the users easier so that they have an easily interpret-able number they can report. We could do the following:
$doubling
$halving
and multiply them by -1Yup yup yup. I agree with all the above! :)
I'll have a go at this for the next next version of incidence. My priority right now is to get a working version on CRAN ASAP for #95 .
expect_equal_to_reference()
that derives from a bug intestthat
Closes #28.