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

Issue 735: Fix truncation slicing when t < truncation #736

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Aug 7, 2024

Description

This PR closes #735.

It adds:

  • Fixes PMF slicing when t < max(truncation pmf)
  • Adds tests for this
  • Adds basic documentation for all observation model stan functions

It does not:

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.

Copy link
Contributor

github-actions bot commented Aug 7, 2024

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

  • ✔️default: 34.7s -> 32.6s [-16.5%, +4.71%]
  • ✔️no_delays: 39.4s -> 42.4s [-2.87%, +18.22%]
  • ✔️random_walk: 11.6s -> 10.6s [-33.01%, +14.58%]
  • ✔️stationary: 23.9s -> 18.6s [-56.38%, +12.16%]
  • ✔️uncertain: 49.8s -> 52s [-11.3%, +20.22%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs seabbs requested a review from sbfnk August 7, 2024 12:52
@seabbs seabbs changed the title Fix truncation slicing when t < truncation Issue 735: Fix truncation slicing when t < truncation Aug 7, 2024
@seabbs seabbs marked this pull request as ready for review August 7, 2024 12:52
@seabbs seabbs enabled auto-merge August 7, 2024 13:01
sbfnk
sbfnk previously approved these changes Aug 7, 2024
Copy link
Contributor

@sbfnk sbfnk left a comment

Choose a reason for hiding this comment

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

Great!

tests/testthat/test-stan-truncate.R Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Aug 7, 2024

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

  • ✔️default: 40.5s -> 31.8s [-53.43%, +10.38%]
  • ✔️no_delays: 44.4s -> 38.2s [-31.73%, +3.51%]
  • ✔️random_walk: 10.2s -> 9.69s [-11.97%, +2.72%]
  • ✔️stationary: 18.5s -> 19s [-12.28%, +18.18%]
  • ✔️uncertain: 49.4s -> 50.1s [-11.1%, +13.94%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs
Copy link
Contributor Author

seabbs commented Aug 7, 2024

Hmm how odd this works locally for me...

Copy link
Contributor

github-actions bot commented Aug 7, 2024

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

  • ✔️default: 32.4s -> 32.7s [-15.73%, +17.45%]
  • ✔️no_delays: 54.6s -> 46.3s [-75.01%, +44.6%]
  • ✔️random_walk: 10.2s -> 10.4s [-17.42%, +21.59%]
  • ✔️stationary: 18.6s -> 19.8s [-13.45%, +26.56%]
  • ✔️uncertain: 53.8s -> 56.2s [-14.4%, +23.12%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor

sbfnk commented Aug 7, 2024

Hmm how odd this works locally for me...

Yes, strange, seems to call base::truncate over the loaded stan function for some reason.

@seabbs
Copy link
Contributor Author

seabbs commented Aug 7, 2024

Maybe its the order things are loaded. I've tried to hack around it by renaming the stan function just after its exposed. Not ideal but good enough I think.

@seabbs seabbs requested a review from sbfnk August 7, 2024 16:50
Copy link
Contributor

github-actions bot commented Aug 7, 2024

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

  • ✔️default: 31.9s -> 1.49m [-219.01%, +578.85%]
  • ✔️no_delays: 42.3s -> 42.3s [-13%, +12.82%]
  • 🚀random_walk: 10.3s -> 9.47s [-16.24%, -0.12%]
  • ✔️stationary: 20.8s -> 18.8s [-21.69%, +2.55%]
  • ✔️uncertain: 54.9s -> 50.9s [-21.19%, +6.57%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor

sbfnk commented Aug 8, 2024

I'm at loss at what's going on here - can't reproduce this locally either (even e.g. running testthat::test_local() in a fresh session, which should be equivalent).

We could rename the function which would be annoying but perhaps the easiest option, or ditch the tests, or try to understand what exactly is going on here (e.g. print the return values of the rstan::expose_stan_functions call).

@seabbs
Copy link
Contributor Author

seabbs commented Aug 8, 2024

We could rename the function which would be annoying but perhaps the easiest option

So I think we want tests over anything else so voted here to change the name to avoid the collision. Not ideal at all though obviously

Copy link
Contributor

github-actions bot commented Aug 8, 2024

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

  • ✔️default: 32.5s -> 37.9s [-2.9%, +36.47%]
  • ✔️no_delays: 43.8s -> 41.6s [-19.58%, +9.54%]
  • ✔️random_walk: 9.5s -> 10.9s [-15.64%, +44.98%]
  • ✔️stationary: 20.4s -> 18.8s [-22.9%, +7.13%]
  • ✔️uncertain: 56.6s -> 54.9s [-21.08%, +15.03%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@seabbs
Copy link
Contributor Author

seabbs commented Aug 8, 2024

@jamesmbaazam as @sbfnk is on holidays if you get chance to give this a look that would be great. I think its mostly there. I am unhappy about the need to name change truncate but am also not sure its worth the effort of dealing with blocking the base::truncate issue.

@seabbs
Copy link
Contributor Author

seabbs commented Aug 8, 2024

Note this PR also starts adding docs as in #740 and improves stan unit test coverage (no way I know of getting some like code coverage for this).

@jamesmbaazam
Copy link
Contributor

Thanks for the fix, Sam. I'm taking a look now.

NEWS.md Outdated Show resolved Hide resolved
jamesmbaazam
jamesmbaazam previously approved these changes Aug 9, 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 for fixing this @seabbs. It looks good to me.

Co-authored-by: James Azam <james.azam@lshtm.ac.uk>
@seabbs seabbs disabled auto-merge August 9, 2024 15:19
@seabbs seabbs merged commit 22e5b22 into main Aug 9, 2024
11 checks passed
@seabbs seabbs deleted the issue735 branch August 9, 2024 15:19
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.

Truncation adjustment does not work as expected if truncation vector is too long
3 participants