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

Not for merge - benchmark summary(<epichains_tree>) vs simulate_summary() #212

Closed
wants to merge 6 commits into from

Conversation

jamesmbaazam
Copy link
Member

This PR sets up a benchmark of two workflows that currently exist in the package for obtaining chain summaries:

  1. Run summary(<epichains_tree>), where <epichains_tree> is obtained from a run of simulate_chains().
  2. Run simulate_summary() with the same parameters passed to simulate_chains().

The benchmarks will help us decide on whether to keep or delete simulate_summary() as it is currently duplicated code that might become a maintenance burden.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (5e6fd49) to head (38e67ac).

❗ Current head 38e67ac differs from pull request most recent head 64d9948. Consider uploading reports for the commit 64d9948 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #212   +/-   ##
=======================================
  Coverage   98.78%   98.78%           
=======================================
  Files           8        8           
  Lines         574      574           
=======================================
  Hits          567      567           
  Misses          7        7           

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

@jamesmbaazam
Copy link
Member Author

@sbfnk The benchmark reveals that simulate_summary() is faster. I don't think that there is a massive difference though.

summary_benchmarks

@jamesmbaazam jamesmbaazam marked this pull request as ready for review March 8, 2024 11:58
benchmarks.R Outdated Show resolved Hide resolved
@jamesmbaazam
Copy link
Member Author

Closing this PR as stale. Will reopen in the discussion about removing simulate_chain_stats() becomes active again.

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.

2 participants