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

Directly calculated growth #610

Merged
merged 7 commits into from
Mar 22, 2024
Merged

Directly calculated growth #610

merged 7 commits into from
Mar 22, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Mar 12, 2024

Description

This PR finishes splits out the direct growth rate calculation from #345 in order to make finishing that work more manageable. It was originally proposed in #213.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

@sbfnk sbfnk force-pushed the directly-calculated-growth branch from 18760dc to 0780ec2 Compare March 12, 2024 17:24
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 19b5707 is merged into main:

  • ✔️default: 41.3s -> 34.2s [-44.04%, +9.41%]
  • ✔️no_delays: 40.1s -> 39.4s [-11.29%, +7.68%]
  • ✔️random_walk: 10s -> 9.73s [-15.96%, +10.57%]
  • ✔️stationary: 19.8s -> 19.7s [-6.91%, +5.99%]
  • ✔️uncertain: 52.1s -> 51.7s [-21.85%, +20.38%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 19b5707 is merged into main:

  • ✔️default: 32.5s -> 32.1s [-11.51%, +8.79%]
  • ✔️no_delays: 38.5s -> 48.5s [-35.52%, +87.14%]
  • ✔️random_walk: 10s -> 10.8s [-18.25%, +33.23%]
  • ✔️stationary: 20.3s -> 24.8s [-41.81%, +86.83%]
  • ✔️uncertain: 55.1s -> 56.6s [-12.59%, +17.87%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk enabled auto-merge (squash) March 12, 2024 23:01
@sbfnk sbfnk requested a review from seabbs March 14, 2024 08:51
@sbfnk sbfnk force-pushed the directly-calculated-growth branch from 8f440bd to a16e9bd Compare March 15, 2024 09:35
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3b3b2a7 is merged into main:

  • ✔️default: 38s -> 37.5s [-14.93%, +11.85%]
  • ✔️no_delays: 44.7s -> 40s [-23.74%, +2.73%]
  • ✔️random_walk: 10.8s -> 11.6s [-3.72%, +17.64%]
  • ✔️stationary: 21.1s -> 21.4s [-10.32%, +12.34%]
  • ✔️uncertain: 1.02m -> 57.4s [-34.97%, +21.53%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3b3b2a7 is merged into main:

  • ✔️default: 37.1s -> 37.4s [-14.73%, +16.38%]
  • ✔️no_delays: 41.9s -> 43.7s [-14.94%, +23.4%]
  • ❗🐌random_walk: 10.1s -> 11.3s [+1.88%, +21.21%]
  • ❗🐌stationary: 18.9s -> 21.2s [+1.9%, +22.6%]
  • ✔️uncertain: 54.5s -> 53.3s [-20.16%, +15.89%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

NEWS.md Outdated Show resolved Hide resolved
seabbs
seabbs previously approved these changes Mar 22, 2024
Copy link
Contributor

@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.

Yup looks good. Just need a news file spelling update

Co-authored-by: Sam Abbott <s.e.abbott12@gmail.com>
@sbfnk sbfnk enabled auto-merge (squash) March 22, 2024 20:51
@sbfnk sbfnk disabled auto-merge March 22, 2024 20:51
@sbfnk sbfnk merged commit 93c072d into main Mar 22, 2024
10 checks passed
@sbfnk sbfnk deleted the directly-calculated-growth branch March 22, 2024 20:51
sbfnk added a commit that referenced this pull request May 3, 2024
* switch to directly calculated growth rate

* remove superseded function

* add news item

* avoid conversion

* add skips

* add test for exp(r)==R with fixed gt of 1 day

* fix typo and add reviewer

Co-authored-by: Sam Abbott <s.e.abbott12@gmail.com>

---------

Co-authored-by: seabbs <s.e.abbott12@gmail.com>
sbfnk added a commit that referenced this pull request May 3, 2024
* switch to directly calculated growth rate

* remove superseded function

* add news item

* avoid conversion

* add skips

* add test for exp(r)==R with fixed gt of 1 day

* fix typo and add reviewer

Co-authored-by: Sam Abbott <s.e.abbott12@gmail.com>

---------

Co-authored-by: seabbs <s.e.abbott12@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants