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.

Sign up for GitHub

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

Jump to bottom

Issue 350: Optimise stan via profiling #501

Merged
merged 19 commits into from
Aug 22, 2024
Merged

Issue 350: Optimise stan via profiling #501

merged 19 commits into from
Aug 22, 2024

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Aug 19, 2024 •
edited
Loading

Description

This PR is related to #350 but doesn't close it as there is more work to do.

Using profiling I have identified areas that are bottlenecking the code (mostly expected_obs) and performance tuned with the slight cost of readability.

I have also looked at the trajectory and posteriors and identified that our diffuse prior on the overdispersion may lead to some significant slow down. Given we have low levels of belief in most settings I propose we moderately tigthen it. In terms of posterior I have seen no impact of this.

I have also gone through all the case studies and examples and checked the adapt_delta and max_treedepth settings. In most cases I have been able to relax these with no diagnostic issues.

Tagged a few reviewers who may be interested in taking a look. @zsusswein may also be interested. @jessalynnsebastian I pinged you as I edited some of the tests and docs for expected_obs() in this PR which you recently touched.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@seabbs seabbs force-pushed the stan-optimistions branch from 5ecde4b to 4e3073c Compare August 19, 2024 18:27
@seabbs seabbs enabled auto-merge August 19, 2024 18:32
@seabbs seabbs force-pushed the stan-optimistions branch from 3973f4d to f766ed7 Compare August 19, 2024 19:20
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@epinowcast epinowcast deleted a comment from github-actions bot Aug 20, 2024
@seabbs seabbs force-pushed the stan-optimistions branch from b55257c to 54b95a4 Compare August 20, 2024 16:24
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@seabbs seabbs force-pushed the stan-optimistions branch from 54b95a4 to 3c63681 Compare August 21, 2024 12:59
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@seabbs @jessalynnsebastian
Update inst/stan/functions/expected_obs.stan
c5290e4 
Co-authored-by: Jessalyn Sebastian <116767845+jessalynnsebastian@users.noreply.github.com>
@github-actions GitHub Actions
Copy link
Contributor

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

  • 🚀day_of_week_model: 38.5s -> 34.9s [-11.81%, -7.11%]
  • 🚀latent_renewal_model: 37.1s -> 26.7s [-31.46%, -24.45%]
  • 🚀missingness_model: 1.36m -> 1.29m [-9.96%, -0.06%]
  • 🚀multi_group_latent_renewal_model: 59.4s -> 53.1s [-17.7%, -3.3%]
  • ✔️preprocessing: 499ms -> 497ms [-2.63%, +1.93%]
  • ✔️simple_model: 9s -> 4.2s [-138.85%, +32.09%]
  • 🚀simple_negbin_model_with_pp: 5.16s -> 4.49s [-22.69%, -3.49%]
    These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

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

  • 🚀day_of_week_model: 39.2s -> 35.8s [-13.69%, -3.7%]
  • 🚀latent_renewal_model: 36.4s -> 28.3s [-29.71%, -15.01%]
  • ✔️missingness_model: 1.32m -> 1.3m [-5.95%, +2.76%]
  • 🚀multi_group_latent_renewal_model: 59.7s -> 52.5s [-14.81%, -9.36%]
  • ✔️preprocessing: 503ms -> 507ms [-0.22%, +1.52%]
  • 🚀simple_model: 5.26s -> 4.12s [-37.66%, -5.89%]
  • ✔️simple_negbin_model_with_pp: 5.2s -> 4.4s [-37.38%, +6.78%]
    These benchmarks are based on package examples which are available here. Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@seabbs seabbs added this pull request to the merge queue Aug 22, 2024
jessalynnsebastian
jessalynnsebastian approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@jessalynnsebastian jessalynnsebastian left a comment

Choose a reason for hiding this comment

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

All looks good to me after those minor changes

Merged via the queue into main with commit 1948ea1 Aug 22, 2024
11 checks passed
@seabbs seabbs deleted the stan-optimistions branch August 22, 2024 18:29
@seabbs
Copy link
Collaborator Author

seabbs commented Aug 23, 2024

Going to monitor this to see if we see any performance impacts from the change in phi prior

@seabbs seabbs mentioned this pull request Aug 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessalynnsebastian jessalynnsebastian jessalynnsebastian approved these changes

@adrian-lison adrian-lison Awaiting requested review from adrian-lison

@medewitt medewitt Awaiting requested review from medewitt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@seabbs @jessalynnsebastian