search

kidusasfaw / spatPomp

R package for statistical inference of spatiotemporal partially observed Markov processes
GNU General Public License v3.0
11 stars 10 forks source link

fix bugs in iubf when faced unintended input #28

Closed Koohoko closed 2 months ago

Koohoko commented 3 months ago

Hi Developers,

Recently I have been using iubf for optimising parameters in a spatPomp model. I find some codes in the iubf function did not behave as expected with some particular input. Here I propose changes in a few lines in the R/iubf.R to fix potential bugs.

  1. log_cond_densities[neighbor_u, ] on this line will cause "out of bounds" error when Nrep_per_param = 1. So I added a check for this, otherwise we may consider forcing Nrep_per_param to be greater than 1.

  2. The def_resample on this line can result in a NULL vector (vector of length 0), if all param_resamp_log_weights are the same. This can happen e.g. for $t_0$, when all observations are at the initial condition and adjusting parameters does have effect on the dmeasure results. A def_resample of length 0 will cause error when sampling it on this line. To fix this, I added a check testing the length of def_resample.

  3. Still for the same code chunk as mention in the previous point, there is a special case need to be addressed when length(def_resample) = 1. The sample function will behave differently if the input is an integer of length 1, comparing to when usually we expect the input def_resample is a vector of length > 1. For example when def_resample equals to an index 3, we want resampling $3$, rather than resampling ${1, 2, 3}$, for multiple times. To fix this, I added a special case for length(def_resample) = 1 here.

Please review if this can be helpful. Thank you so much again for this package.

Best, Haogao

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.92%. Comparing base (0332b11) to head (bef1b1e). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #28 +/- ## =========================================== - Coverage 100.00% 99.92% -0.08% =========================================== Files 54 54 Lines 4858 4864 +6 =========================================== + Hits 4858 4860 +2 - Misses 0 4 +4 ```
ionides commented 2 months ago

Hi Haogao, thanks for this collection of improvements. I have merged them and will add unit tests shortly.