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

Switching to broadcasting random walk #747

Merged
merged 14 commits into from
Aug 29, 2024
Merged

Switching to broadcasting random walk #747

merged 14 commits into from
Aug 29, 2024

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Aug 15, 2024

Description

Following on from #746 this PR switches to broadcasting for the random walk parameterizations. It also adds docs and tests.

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.

@seabbs seabbs enabled auto-merge August 15, 2024 16:39

This comment was marked as outdated.

This comment was marked as outdated.

Copy link
Contributor

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

  • ✔️default: 42.5s -> 41.4s [-24.51%, +19.32%]
  • ✔️no_delays: 45.4s -> 48.9s [-8.07%, +23.47%]
  • ✔️random_walk: 9.93s -> 9.54s [-16.52%, +8.61%]
  • ✔️stationary: 23.6s -> 19.7s [-50.29%, +16.86%]
  • ✔️uncertain: 1.19m -> 1.19m [-32.79%, +33.36%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

NEWS.md Outdated Show resolved Hide resolved
jamesmbaazam
jamesmbaazam previously approved these changes Aug 28, 2024
Copy link
Contributor

@jamesmbaazam jamesmbaazam left a comment

Choose a reason for hiding this comment

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

Thanks @seabbs. LGTM. Thanks for adding the tests and documentation.

To confirm that I understood everything here. This was more of a refactoring exercise to use more elegant stan tricks right? I noticed most of the right hand side code is a better rewrite of the deleted code using what we would call vectorization in R parlance.

Co-authored-by: James Azam <[email protected]>
@seabbs
Copy link
Contributor Author

seabbs commented Aug 28, 2024

yes and to ideally reduce compilation and run time (which I think it does a tiny bit)

Copy link
Contributor

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

  • ✔️default: 45.1s -> 41.8s [-24.03%, +9.1%]
  • ✔️no_delays: 48.4s -> 50.8s [-20.71%, +30.28%]
  • 🚀random_walk: 10s -> 8.72s [-25.79%, -0.65%]
  • ✔️stationary: 20.1s -> 20.2s [-18.99%, +20.08%]
  • ✔️uncertain: 1.16m -> 1.06m [-17.68%, +1.32%]
    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 8457a9e is merged into main:

  • 🚀default: 51.6s -> 44.2s [-25.39%, -3.16%]
  • ✔️no_delays: 52.1s -> 50.4s [-18.4%, +11.55%]
  • ✔️random_walk: 9.89s -> 9.79s [-10.02%, +7.99%]
  • ✔️stationary: 20.9s -> 20.3s [-14.35%, +8.79%]
  • ✔️uncertain: 1.07m -> 1.17m [-13.96%, +33.69%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs seabbs added this pull request to the merge queue Aug 29, 2024
Merged via the queue into main with commit 39cdaff Aug 29, 2024
13 checks passed
@seabbs seabbs deleted the broadcast-rw branch August 29, 2024 13:39
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