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

Export LFMCMC #27

Merged
merged 73 commits into from
Nov 5, 2024
Merged

Export LFMCMC #27

merged 73 commits into from
Nov 5, 2024

Conversation

apulsipher
Copy link
Contributor

@apulsipher apulsipher commented Oct 15, 2024

This pull request introduces several significant updates, including the addition of a new Likelihood-Free Markov Chain Monte Carlo (LFMCMC) functionality, various improvements to the codebase, and updates to documentation and configuration files.

New Features:

  • Likelihood-Free Markov Chain Monte Carlo (LFMCMC):
    • Added the LFMCMC class and associated methods for performing LFMCMC simulations (R/LFMCMC.R).
    • Introduced corresponding C++ functions for LFMCMC (R/cpp11.R).
    • Updated NAMESPACE to export new LFMCMC-related functions (NAMESPACE). [1] [2] [3] [4]

Codebase Improvements:

  • Docker and VSCode Configuration:
    • Updated Dockerfile to install debugging tools (.devcontainer/Dockerfile).
    • Added cinttypes to VSCode settings (.vscode/settings.json).
  • Makefile Enhancements:
    • Fixed a typo in the docs target and added a new dev target for building without vignettes (Makefile).

Documentation and Configuration:

  • Documentation Updates:
    • Corrected the roxygenize function call in the Makefile (Makefile).
  • Project Configuration:
    • Updated project settings to avoid cleaning before installing (epiworldR.Rproj).

Minor Enhancements:

  • C++ Enhancements:
    • Added new methods to the DataBase class for transition matrices and recording transmissions (inst/include/epiworld/database-bones.hpp, inst/include/epiworld/database-meat.hpp). [1] [2] [3] [4] [5]
    • Updated LFMCMC class to use std::shared_ptr for the random number engine (inst/include/epiworld/math/lfmcmc/lfmcmc-bones.hpp).
    • Introduced a new distributions.hpp file for mathematical distributions (inst/include/epiworld/math/distributions.hpp).

Version Update:

  • Version Bump:
    • Incremented the minor and patch version numbers (inst/include/epiworld/epiworld.hpp).

These changes collectively enhance the functionality, maintainability, and usability of the codebase.

@apulsipher apulsipher linked an issue Oct 15, 2024 that may be closed by this pull request
3 tasks
@apulsipher apulsipher marked this pull request as draft October 15, 2024 15:24
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 19.07%. Comparing base (b9a29fc) to head (08e677b).

Files with missing lines Patch % Lines
R/LFMCMC.R 0.00% 3 Missing ⚠️
src/lfmcmc.cpp 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   19.14%   19.07%   -0.07%     
==========================================
  Files          34       36       +2     
  Lines        1776     1782       +6     
==========================================
  Hits          340      340              
- Misses       1436     1442       +6     

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

@apulsipher
Copy link
Contributor Author

@gvegayon I implemented set_kernel_fun and set_proposal_fun. For the vignette, I wrote R versions of kernel_fun_uniform and proposal_fun_normal because the gaussian kernel and the norm reflective proposal functions are more complicated and I felt it would clutter the vignette. However, the result of the vignette is now much different from the example in the C++ library. Is that going to be okay? Or should I revert the vignette to have use_proposal_norm_reflective and use_kernel_fun_gaussian instead of the R-defined versions?

@apulsipher apulsipher marked this pull request as ready for review November 4, 2024 20:23
@gvegayon
Copy link
Member

gvegayon commented Nov 4, 2024

I haven't executed the C++ version in a while, so maybe it is not so different now! Anyhow, I think that, for the moment, the only relevant issue is the following warning reported by R CMD check:

Found the following significant warnings:
  ../inst/include/epiworld/math/lfmcmc/lfmcmc-meat-print.hpp:55:36: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'size_t' {aka 'long long unsigned int'} [-Wformat=]

That is on the C++ side, so it would need to be addressed directly on epiworld C++. Can you fix that via a PR (another patch update). Once that's done, I think we can merge 🥳!

Copy link
Member

@gvegayon gvegayon left a comment

Choose a reason for hiding this comment

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

A couple of comments before merging. The main one is to ensure the version in DESCRIPTION matches the version of epiworld.hpp. Once that's addressed, you can merge!

.vscode/settings.json Outdated Show resolved Hide resolved
inst/include/epiworld/epiworld.hpp Show resolved Hide resolved
@apulsipher
Copy link
Contributor Author

@gvegayon Does everything look okay now?

@gvegayon
Copy link
Member

gvegayon commented Nov 5, 2024

Please squash and merge :)

@apulsipher apulsipher merged commit 5b646c4 into main Nov 5, 2024
11 of 13 checks passed
@apulsipher apulsipher deleted the 25-export-lfmcmc branch November 5, 2024 20:11
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.

Export LFMCMC
2 participants