Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

ntorresd
Copy link
Member

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
Copy link

codecov-commenter commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntorresd ntorresd requested a review from Bisaloo January 29, 2024 01:38
@ntorresd ntorresd marked this pull request as ready for review January 29, 2024 01:38
Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ntorresd ntorresd linked an issue Jan 29, 2024 that may be closed by this pull request
@ntorresd
Copy link
Member Author

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 .

- Replace incorrect use of `testthat::expect_equal()` for correct
comparison of `get_prev_expanded_clean` outputs with corresponding
benchmarks.
- Split modelling testing by type of models (constant and time-varying)
Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is very helpful!

For the tolerance specification, this feels more like a stats question so I'll let you double check with others if necessary.

tests/testthat/test_tv_models.R Show resolved Hide resolved
@ntorresd ntorresd requested a review from Bisaloo January 30, 2024 13:12
Co-authored-by: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com>
@ntorresd ntorresd merged commit b09034c into main Jan 30, 2024
6 checks passed
@ntorresd ntorresd deleted the fix-get_prev_expanded_clean branch January 30, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in matrix calculation get_prev_expanded
3 participants