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

allow snapshots shorter than trunc_max #438

Merged
merged 3 commits into from Jul 30, 2023
Merged

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Jul 25, 2023

estimate_truncation currently fails when the any snapshot is shorter than trunc_max. This PR fixes that by provding explicit bounds and vector elements in the appropriate place.

@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 59.3s -> 57.5s [-25.05%, +18.92%]
  •   :ballot_box_with_check:no_delays: 59.1s -> 57.4s [-15.7%, +10%]
  • ❗🐌random_walk: 15.1s -> 17.2s [+2.13%, +24.56%]
  •   :ballot_box_with_check:stationary: 32.5s -> 33.2s [-7.96%, +12.27%]
  •   :ballot_box_with_check:uncertain: 1.22m -> 1.28m [-9.06%, +19.61%]
    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.

All looks good. I would probably phrase this as a new feature vs fixing a bug as the function was never meant to work when snapshots < delays (though this was of course not communicated).

Just needs a dev version bump.

inst/stan/estimate_truncation.stan Outdated Show resolved Hide resolved
@sbfnk sbfnk force-pushed the estimate-truncation-short branch from 63b6fd5 to bff2f15 Compare July 28, 2023 09:49
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

@sbfnk sbfnk force-pushed the estimate-truncation-short branch from bff2f15 to 8bcd8bb Compare July 28, 2023 09:51
@sbfnk
Copy link
Contributor Author

sbfnk commented Jul 28, 2023

I would probably phrase this as a new feature vs fixing a bug

Done in 4bfc01e

@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 1.07m -> 1.01m [-15.21%, +4.51%]
  •   :ballot_box_with_check:no_delays: 58.6s -> 57.2s [-13.85%, +8.95%]
  •   :rocket:random_walk: 17.9s -> 16.8s [-12.67%, -0.19%]
  •   :ballot_box_with_check:stationary: 37.3s -> 37.6s [-9.03%, +10.4%]
  •   :ballot_box_with_check:uncertain: 1.25m -> 1.33m [-12.09%, +24.8%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs
Copy link
Contributor

seabbs commented Jul 28, 2023

Ruh Roh

@sbfnk sbfnk force-pushed the estimate-truncation-short branch from 2ac0e7e to d81e94f Compare July 28, 2023 17:38
@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 46.1s -> 45.7s [-14.08%, +12.18%]
  •   :ballot_box_with_check:no_delays: 44.3s -> 45.8s [-8.47%, +15.65%]
  •   :ballot_box_with_check:random_walk: 14.1s -> 17s [-8.35%, +49.82%]
  •   :ballot_box_with_check:stationary: 29s -> 28.2s [-14.76%, +9.84%]
  •   :ballot_box_with_check:uncertain: 1.12m -> 1.07m [-17.08%, +9.19%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk force-pushed the estimate-truncation-short branch from d81e94f to 9cec97f Compare July 29, 2023 21:31
@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 48.3s -> 51.8s [-6.29%, +21.1%]
  •   :ballot_box_with_check:no_delays: 45.3s -> 48.6s [-4.95%, +19.41%]
  •   :ballot_box_with_check:random_walk: 27.3s -> 14s [-163.96%, +66.79%]
  •   :ballot_box_with_check:stationary: 28.1s -> 29.1s [-5.38%, +12.95%]
  •   :ballot_box_with_check:uncertain: 1.07m -> 1.15m [-7.96%, +22.72%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk force-pushed the estimate-truncation-short branch from 9cec97f to ba060db Compare July 30, 2023 06:50
@github-actions
Copy link

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

  •   :ballot_box_with_check:default: 56.6s -> 57s [-14.51%, +16.12%]
  •   :ballot_box_with_check:no_delays: 57.3s -> 58s [-6.35%, +8.97%]
  • ❗🐌random_walk: 16.4s -> 18.7s [+2.89%, +24.78%]
  •   :ballot_box_with_check:stationary: 36.2s -> 33.2s [-20.01%, +3.29%]
  •   :ballot_box_with_check:uncertain: 1.38m -> 1.46m [-10.74%, +22.36%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk sbfnk merged commit 0d2c67e into main Jul 30, 2023
11 checks passed
@sbfnk sbfnk deleted the estimate-truncation-short branch July 30, 2023 13:16
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.

None yet

2 participants