dajmcdon / rtestim

https://dajmcdon.github.io/rtestim/
Other
4 stars 0 forks source link

Ec/uneven delay cal #27

Closed zcaiElvis closed 1 year ago

zcaiElvis commented 1 year ago

Added support for uneven-spaced x for cross-validation purposes only

A major issue that needs to be addressed for general purposes is: If x = c(1, 2, 3, 4.1, 5), this function creates a new new_x = seq(1, 5, 0.9) and interpolate case counts at new_x that's not in x.

zcaiElvis commented 1 year ago

I did git merge main to make sure I have all updates from the main and the new delay_calculator passed all tests, but the check still fail.

dajmcdon commented 1 year ago

See the error:

Screenshot 2023-02-14 at 12 56 40 PM

Likely, weighted_past_counts contains 0, and you're dividing by it.

zcaiElvis commented 1 year ago

Oh I didn't notice this change. Why are we not using y <- c(rev(seq(2, 6, by = 1)), seq(2, 6, by = 1)) anymore?

dajmcdon commented 1 year ago

Because that example was too simple to catch real issues. The point of examples is 2 fold:

  1. illustrate how to use the function to potential users
  2. (and more importantly) set up some scenarios that must always work.

The point here is to catch 2. Your changes have introduced some bugs.

zcaiElvis commented 1 year ago

Sounds good the new example makes more sense.

This problem in check didn't appear before because the convolve function was missing the parameter type = "open" so the calculation of weighted_past_counts was wrong.

The root cause for the failed check was when observed_counts starts at 0, the first two elements of weighted_past_counts were 0. I made the change to replace observed_counts[1] = 1 if observed_counts[1] == 0