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

Convert duplicated code into helper functions #199

Merged
merged 38 commits into from
Feb 19, 2024
Merged

Conversation

jamesmbaazam
Copy link
Member

@jamesmbaazam jamesmbaazam commented Feb 5, 2024

  • Please check if the PR fulfils these requirements
  • I have read the CONTRIBUTING guidelines
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • It refactors simulate_chains(), simulate_summary(), and likelihood() to remove duplicated code by converting them into internal helper functions follows:

    • Simulation related operations have been modularised into .init_susc_pop(), .sample_possible_offspring(), and
      .get_susceptible_offspring().
    • Input checking is now remitted to 3 functions, .check_sim_args(), which checks the shared arguments of the simulate_* () functions, and 2 smaller functions, .check_statistic_args() which checks stat_max and statistic together in multiple functions, and .check_time_args(), which checks the time-related arguments: generation_time, t0, and tf.
  • Tests and documentation, where necessary, have been added.

  • What is the current behaviour? (You can also link to an open issue here)

This is a refactoring exercise and leads to no user-facing behaviour change.

  • What is the new behaviour (if this is a feature change)?

    This is a refactoring exercise and leads to no user-facing behaviour change.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

  • Other information:

@jamesmbaazam jamesmbaazam changed the title Add and apply function to initialise susceptible pop Convert duplicated code into helper functions Feb 6, 2024
@jamesmbaazam jamesmbaazam marked this pull request as ready for review February 14, 2024 18:41
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. Also

> cyclocomp::cyclocomp(simulate_chains )
[1] 15

so can switch the lintr on again.

R/simulate.r Outdated Show resolved Hide resolved
R/simulate.r Outdated Show resolved Hide resolved
R/simulate.r Outdated Show resolved Hide resolved
R/simulate.r Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
R/helpers.R Outdated Show resolved Hide resolved
inst/WORDLIST Outdated Show resolved Hide resolved
tests/testthat/test-checks.R Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d22fd51) 98.90% compared to head (56c0a74) 98.94%.

❗ Current head 56c0a74 differs from pull request most recent head 32ccc24. Consider uploading reports for the commit 32ccc24 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
+ Coverage   98.90%   98.94%   +0.04%     
==========================================
  Files           8        8              
  Lines         550      571      +21     
==========================================
+ Hits          544      565      +21     
  Misses          6        6              

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

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.

All good. Just one last comment.

R/checks.R Outdated Show resolved Hide resolved
@jamesmbaazam jamesmbaazam merged commit ea1dd16 into main Feb 19, 2024
10 checks passed
@jamesmbaazam jamesmbaazam deleted the fix-code-dup branch February 19, 2024 13:49
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.

Code duplication as a result of splitting chain_sim()
3 participants