Issue 735: Fix truncation slicing when t < truncation

[seabbs]

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:

• Add a warning about sparse PMFs (Add a warning when PMFs are very sparse in the tails #738)
• Add a warning about PMFs that are longer than the vector of observations (Add a warning when the length of delays are longer than the supplied data #739)

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.

[github-actions]

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.

[sbfnk]

Great!

[github-actions]

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]

Hmm how odd this works locally for me...

[github-actions]

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]

Hmm how odd this works locally for me...

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

[seabbs]

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.

[github-actions]

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]

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]

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

[github-actions]

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]

@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]

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]

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

[jamesmbaazam]

Thanks for fixing this @seabbs. It looks good to me.