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

reduce the number of stan models #394

Merged
merged 11 commits into from May 23, 2023
Merged

reduce the number of stan models #394

merged 11 commits into from May 23, 2023

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented May 17, 2023

fixes #393 by combining the models from dist_fit into one stan file and ditching the (unused) tune_inv_gamma model (which could of course be brought back if needed).

@seabbs
Copy link
Contributor

seabbs commented May 17, 2023

tune_inv_gamma model (which could of course be brought back if needed).

I assume you have checked this isn't used?

@sbfnk
Copy link
Contributor Author

sbfnk commented May 17, 2023

I assume you have checked this isn't used?

Yes - I'm not sure it was ever used, at least I couldn't find a use traversing through git blames.

@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 1.06m -> 59.3s [-25.51%, +12.31%]
  •   :ballot_box_with_check:no_delays: 53.8s -> 48.9s [-18.3%, +0.08%]
  •   :ballot_box_with_check:random_walk: 16.3s -> 16.8s [-7.89%, +14.68%]
  •   :ballot_box_with_check:stationary: 40.1s -> 44.2s [-0.35%, +20.55%]
  •   :ballot_box_with_check:uncertain: 1.13m -> 1.09m [-11.66%, +4.24%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

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.

This looks good. I think we need a clean depreciation notice here so ideally that would use lifecycle to report that tune_inv_gamma has been deprecated if called. Even more ideally this could give a link to a gist etc where the code still exists (or the git history) and a note saying to let the authors know if using.

Otherwise this is good to go once it has a news update and a Dev version update.

Nice to speed up package compilation!

@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 43.7s -> 49.1s [-3.38%, +28.06%]
  •   :ballot_box_with_check:no_delays: 39.9s -> 38.7s [-10.51%, +4.31%]
  •   :ballot_box_with_check:random_walk: 12.2s -> 12.5s [-6.2%, +11.12%]
  •   :ballot_box_with_check:stationary: 29.8s -> 31s [-6.12%, +14.17%]
  •   :ballot_box_with_check:uncertain: 48.1s -> 47s [-17.63%, +13.24%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 1.03m -> 1.04m [-12.31%, +14.6%]
  •   :ballot_box_with_check:no_delays: 47.9s -> 50.4s [-8.68%, +19.11%]
  •   :ballot_box_with_check:random_walk: 16.1s -> 19.2s [-33.71%, +73.41%]
  •   :ballot_box_with_check:stationary: 40.9s -> 39.7s [-17.85%, +12.16%]
  •   :ballot_box_with_check:uncertain: 59.5s -> 1m [-21.34%, +23.16%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk requested a review from seabbs May 23, 2023 13:32
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.

LGTM.

@seabbs seabbs merged commit 0c1aef7 into main May 23, 2023
11 checks passed
@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 47.9s -> 47.8s [-15.94%, +15.33%]
  •   :ballot_box_with_check:no_delays: 40.1s -> 38.8s [-10.63%, +4.03%]
  •   :ballot_box_with_check:random_walk: 13s -> 12.1s [-13.84%, +0.01%]
  •   :ballot_box_with_check:stationary: 30.1s -> 29.8s [-12.64%, +10.87%]
  •   :ballot_box_with_check:uncertain: 48.8s -> 52.5s [-5.56%, +20.68%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs seabbs deleted the fewer_models branch May 24, 2023 10:32
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.

reduce number of models to be compiled
2 participants