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

Refactor likelihood functions #58

Merged
merged 57 commits into from
Sep 13, 2023
Merged

Refactor likelihood functions #58

merged 57 commits into from
Sep 13, 2023

Conversation

jamesmbaazam
Copy link
Member

@jamesmbaazam jamesmbaazam commented Sep 6, 2023

This PR closes #48 by ensuring that likelihoods and log-likelihoods are properly calculated. The log argument is removed from offspring_ll() and retained in likelihood() to centralize the (log)likelihood and joint (log)likelihood calculations. The PR also adds tests to check the functionality of the likelihood() function as well as the *_ll() functions to partly address #45.

This PR also fixes a few minor issues, including:

  • Clarifying that loglikelihoods, not likelihoods are returned by the *_ll() family of functions.
  • Adding new examples and improving some old ones.

@jamesmbaazam jamesmbaazam marked this pull request as draft September 6, 2023 17:03
@jamesmbaazam jamesmbaazam changed the title Refactor likelihood function and fix log argument Refactor likelihood functions Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Merging #58 (6b7364c) into main (8561fc0) will increase coverage by 22.28%.
Report is 8 commits behind head on main.
The diff coverage is 91.37%.

❗ Current head 6b7364c differs from pull request most recent head 9e601a1. Consider uploading reports for the commit 9e601a1 to get more accurate results

@@             Coverage Diff             @@
##             main      #58       +/-   ##
===========================================
+ Coverage   39.95%   62.23%   +22.28%     
===========================================
  Files           8        8               
  Lines         413      429       +16     
===========================================
+ Hits          165      267      +102     
+ Misses        248      162       -86     
Files Changed Coverage Δ
R/checks.R 53.33% <0.00%> (+26.66%) ⬆️
R/epichains.R 0.00% <ø> (ø)
R/simulate.r 82.46% <66.66%> (+5.84%) ⬆️
R/likelihood.R 100.00% <100.00%> (+100.00%) ⬆️
R/stat_likelihoods.R 100.00% <100.00%> (+42.85%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jamesmbaazam jamesmbaazam added bug Something isn't working enhancement New feature or request pkg_infrastructure labels Sep 7, 2023
@jamesmbaazam jamesmbaazam marked this pull request as ready for review September 11, 2023 18:58
@jamesmbaazam jamesmbaazam requested a review from sbfnk September 11, 2023 18:59
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Great stuff - left a few comments.

R/likelihood.R Show resolved Hide resolved
R/likelihood.R Outdated Show resolved Hide resolved
R/likelihood.R Outdated Show resolved Hide resolved
R/likelihood.R Outdated Show resolved Hide resolved
R/likelihood.R Outdated Show resolved Hide resolved
R/likelihood.R Show resolved Hide resolved
R/likelihood.R Show resolved Hide resolved
R/likelihood.R Show resolved Hide resolved
R/stat_likelihoods.R Outdated Show resolved Hide resolved
tests/testthat/test-stat_likelihoods.R Show resolved Hide resolved
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Great

R/likelihood.R Outdated Show resolved Hide resolved
@jamesmbaazam
Copy link
Member Author

Thanks for your thorough review, @sbfnk. Really enjoyed the process. Will merge this after the checks pass.

@jamesmbaazam jamesmbaazam merged commit af04206 into main Sep 13, 2023
9 checks passed
@jamesmbaazam jamesmbaazam deleted the fix_likelihood_func branch September 13, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request refactoring
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Fix log argument not being passed to offspring_ll
3 participants