search

epiverse-trace / serofoi

Estimates the Force-of-Infection of a given pathogen from population based sero-prevalence studies
https://epiverse-trace.github.io/serofoi/
Other
17 stars 4 forks source link

fix(bug): correct exposure_expanded matrix calculation avoiding loops #153

Closed ntorresd closed 7 months ago

ntorresd commented 7 months ago

This PR fixes the bug described in #145 . The right exposure triangular matrix with respect to the anti-diagonal is computed avoiding for loops by:

ly <- NCOL(foi_expanded)
exposure_expanded <- matrix(0, nrow = ly, ncol = ly)
exposure_expanded[apply(
  lower.tri(exposure_expanded, diag = TRUE),
  1, rev
  )] <- 1
codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (044dbb8) 70.07% compared to head (b5b6002) 68.12%.

:exclamation: Current head b5b6002 differs from pull request most recent head 22244db. Consider uploading reports for the commit 22244db to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #153 +/- ## ========================================== - Coverage 70.07% 68.12% -1.95% ========================================== Files 10 10 Lines 1751 1754 +3 ========================================== - Hits 1227 1195 -32 - Misses 524 559 +35 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ntorresd commented 7 months ago

This issue was not detected by tests so can we add tests to ensure it doesn't pop up again?

The reason why the tests did not detect this problem was because we were using incorrectly the function expect_equal(). I replaced it by a combination of expect_true and dplyr::near(), which allows for vector comparison with specific tolerance. I'm setting the tolerance as half the confidence interval size for each age as a heuristic threshold that I find reasonable. Please let me know if you can think of a better criteria. This one seems to perform well for the current models even without setting a seed and fails when using the bugged version of get_prev_expanded() for both time-varying models.

I decided to split the tests for the constant and the time-varying models into separate files in preparation for future tests that will be incorporated once we implement the additional time-varying models (#69 ) and age-vaying models (#74 ), which may be relevant for #85 .