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

Issue #477: Add analytical likelihoods to R functionality #540

Merged
merged 20 commits into from
Feb 26, 2025
Merged

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Feb 19, 2025

Description

This PR closes #477. It also closes #538 and #526. It is a draft and so not suitable for review/merging.

This PR:

  • Fixes a vector length issue for cens that was causing an issue for some likelihood calls
  • Adds stan side support for fitting all distributions that primarycensored supports
  • Adds support for R side analytical likelihoods (so far for just lognormal and gamma). Note this support has to be added here manually as new analytical solutions are added to primarycensored. This is unfortunate but I can't find a workaround.

I have checked the support for priorsense and loo. Without the analytical likelihoods the code in this PR enables these functions to run but with 10 cores loo still takes 5 minutes and priorsense runs for longer than I wanted to wait and see. With the analytical solutions enabled (this also side steps #476 hence the big speed up) loo takes a few seconds and priorsense takes less than 30 seconds for me (the code in the FAQ vignette).

Work to do:

  • Add support for Weibull analytical solutions
  • see if can reduce code duplication
  • docs sweep
  • fill testing gaps (including priorsense and loo)
  • Add integration tests
  • Turn on the eval false FAQ code
  • linting

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs
Copy link
Contributor Author

seabbs commented Feb 26, 2025

see if can reduce code duplication

Saving this to address with a more well-formed solution in #476.

@seabbs seabbs marked this pull request as ready for review February 26, 2025 17:02
@epinowcast epinowcast deleted a comment from codecov bot Feb 26, 2025
Copy link
Contributor Author

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Another quite big PR with some masking linting and formatting fixes but all looks good to me and test coverage of the changes is highly.

Going to resolve the pre-commit issues which are due to Air and styler having a conflict in its own PR.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

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

Project coverage is 95.59%. Comparing base (f5eead1) to head (453e2b9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
R/formula.R 0.00% 2 Missing ⚠️
R/simulate.R 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
+ Coverage   94.38%   95.59%   +1.20%     
==========================================
  Files          17       17              
  Lines         837      998     +161     
==========================================
+ Hits          790      954     +164     
+ Misses         47       44       -3     

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

@seabbs seabbs merged commit bdfbaad into main Feb 26, 2025
10 of 11 checks passed
@seabbs seabbs deleted the seabbs/issue477 branch February 26, 2025 17:26
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.

priorsense integration not working Update epidist_gen_log_lik to use primarycensored analytical solutions
1 participant