Issue #362: Reduce dependence on `cmdstanr` #424

athowes · 2024-11-06T12:36:24Z

Description

This draft PR explores #362.

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.

R/fit.R

athowes · 2024-11-11T12:02:20Z

Fitting a model with cmdstanr then it contains the posterior draws. Fitting it with rstan then it doesn't. Need to change our functionality which interfaces with e.g. brms::prepare_predictions to be agnostic. How does brms achieve this? If it's fit with rstan then there is an extra step?

athowes · 2024-11-13T14:27:27Z

@seabbs to look at then discuss.

seabbs · 2024-11-13T18:46:41Z

@athowes as you can tell from the commits I have started to look at this will circle back once I have made more progress

seabbs · 2024-11-14T13:59:24Z

@athowes this is ready for review.

It completes your work removing the dependency on cmdstanr.

It also updates the tests to use fits from the setup script, and updates most of the vignettes to use cmdstanr. Finally it renames the epidist containing script to match the name and fixes the outstanding CRAN notes.

seabbs

LGTM

R/epidist.R

athowes

Nice!

Main larger thing is can we delete the saving of the data file seeing as it's not used now?

With regard to this change, perhaps it's going to slow down development sometimes (as I rerun the test preparation script) but I'm OK with it given the benefits.

I'm also confused why we are getting rid of tests of the model compiling.

inst/generate_examples.R

tests/testthat/test-diagnostics.R

tests/testthat/test-int-direct_model.R

tests/testthat/test-int-latent_individual.R

vignettes/approx-inference.Rmd

vignettes/ebola.Rmd

vignettes/epidist.Rmd

seabbs · 2024-11-14T14:58:23Z

I'm also confused why we are getting rid of tests of the model compiling.

because the test suite is super super slow and we don't need them as we are checking the syntax and we know we can compile models more generally in the fitting

seabbs · 2024-11-14T15:06:10Z

Main larger thing is can we delete the saving of the data file seeing as it's not used now?

I think maybe if we don't need it for examples (which are currently quite missing) probably a new issue though.

With regard to this change, perhaps it's going to slow down development sometimes (as I rerun the test preparation script) but I'm OK with it given the benefits.

I think its the same or fewer number of model fits so the test suite should actually be faster

…nr and not rstan

Co-authored-by: Adam Howes <[email protected]>

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 729e385075e181a2c7d9a716e776ac504d08a658 [formerly 70e78c0742b69012eb57ad58c5d92e39d25c45ad] Former-commit-id: 38ff128853245a2fba7acdbe49951aabc476647e

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 4afb056 Former-commit-id: 764790be7d4ec49bce89e458a96556e91a472cb2

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 4afb056 Former-commit-id: 764790be7d4ec49bce89e458a96556e91a472cb2

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 4afb056 Former-commit-id: 764790be7d4ec49bce89e458a96556e91a472cb2

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 729e385075e181a2c7d9a716e776ac504d08a658 [formerly 70e78c0742b69012eb57ad58c5d92e39d25c45ad] Former-commit-id: 38ff128853245a2fba7acdbe49951aabc476647e

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 729e385075e181a2c7d9a716e776ac504d08a658 [formerly 70e78c0742b69012eb57ad58c5d92e39d25c45ad] Former-commit-id: 38ff128853245a2fba7acdbe49951aabc476647e Former-commit-id: 9fc612e

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 4afb056 Former-commit-id: 764790be7d4ec49bce89e458a96556e91a472cb2 Former-commit-id: 0de625c0e819bd977b6691ec528c385cbbd34615 [formerly 32c2660] Former-commit-id: 67be8349533318a2c020276788ccdfe2dd84e106

@jamesmbaazam

* Switch default to rstan rather than cmdstanr * Import BH * Use match.arg as suggested by @jamesmbaazam * Add RcppEigen to Suggests * Change backend to cmdstanr in approximate inference vignette * Use cmdstanr for Laplace test * Remove erroneous output_dir arg * drop backend entirely as an arg * remove cmdstanr using vignettes from cran check * update cmdstan install instructions * update epidist docs * update approx-inference vignette to make it clear we are using cmdstanr and not rstan * switch ebola vignette over to cmdstanr * update test fit returns * update more fit uses in test * fix helper function * bring silence to laplace * fix epidist * get tests passing * fix CRAN check * use fewer cores * drop additional deps * drop duplicate prepping * try and use linking with instead of imports * Update vignettes/epidist.Rmd Co-authored-by: Adam Howes <[email protected]> * Update tests/testthat/test-int-latent_individual.R Co-authored-by: Adam Howes <[email protected]> * Update inst/generate_examples.R Co-authored-by: Adam Howes <[email protected]> --------- Co-authored-by: Sam <[email protected]> Former-commit-id: 4afb056 Former-commit-id: 764790be7d4ec49bce89e458a96556e91a472cb2 Former-commit-id: 0de625c0e819bd977b6691ec528c385cbbd34615 [formerly 32c2660] Former-commit-id: 67be8349533318a2c020276788ccdfe2dd84e106

jamesmbaazam reviewed Nov 7, 2024

View reviewed changes

R/fit.R Outdated Show resolved Hide resolved

athowes linked an issue Nov 7, 2024 that may be closed by this pull request

Reduce dependence on cmdstanr given it's suggested not required #362

Closed

This comment was marked as outdated.

Sign in to view

athowes mentioned this pull request Nov 13, 2024

Skip tests with fit data loaded on CRAN? #342

Closed

athowes force-pushed the reduce-cmdstanr-dep branch from 2db875e to 03f4971 Compare November 13, 2024 16:21

seabbs marked this pull request as ready for review November 14, 2024 13:15

seabbs previously approved these changes Nov 14, 2024

View reviewed changes

athowes commented Nov 14, 2024

View reviewed changes

R/epidist.R Show resolved Hide resolved

athowes commented Nov 14, 2024

View reviewed changes

seabbs dismissed their stale review via 713eacc November 14, 2024 14:58

athowes and others added 14 commits November 14, 2024 15:06

Switch default to rstan rather than cmdstanr

790cc34

Import BH

501d65b

Use match.arg as suggested by @jamesmbaazam

4b21d38

Add RcppEigen to Suggests

e95ab45

Change backend to cmdstanr in approximate inference vignette

db0278d

Use cmdstanr for Laplace test

409d088

Remove erroneous output_dir arg

f34f7d7

drop backend entirely as an arg

89243a7

remove cmdstanr using vignettes from cran check

056586a

update cmdstan install instructions

ab0d9dc

update epidist docs

05c4e89

update approx-inference vignette to make it clear we are using cmdsta…

74cb2fb

…nr and not rstan

switch ebola vignette over to cmdstanr

1ec8893

update test fit returns

fd50d2e

seabbs and others added 13 commits November 14, 2024 15:06

update more fit uses in test

3c5b4ef

fix helper function

b61d983

bring silence to laplace

b01485c

fix epidist

61e2ae3

get tests passing

1dd80c9

fix CRAN check

118c609

use fewer cores

42463fc

drop additional deps

404bfd4

drop duplicate prepping

e543b96

try and use linking with instead of imports

b44f84f

Update vignettes/epidist.Rmd

0c4a2bd

Co-authored-by: Adam Howes <[email protected]>

Update tests/testthat/test-int-latent_individual.R

74235c6

Co-authored-by: Adam Howes <[email protected]>

Update inst/generate_examples.R

c14152d

Co-authored-by: Adam Howes <[email protected]>

seabbs force-pushed the reduce-cmdstanr-dep branch from f8f4611 to c14152d Compare November 14, 2024 15:06

seabbs merged commit 4afb056 into main Nov 14, 2024
5 checks passed

seabbs deleted the reduce-cmdstanr-dep branch November 14, 2024 15:06

github-actions bot restored the reduce-cmdstanr-dep branch November 14, 2024 15:08

athowes deleted the reduce-cmdstanr-dep branch November 14, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #362: Reduce dependence on `cmdstanr` #424

Issue #362: Reduce dependence on `cmdstanr` #424

athowes commented Nov 6, 2024 •

edited by seabbs

Loading

athowes commented Nov 11, 2024

This comment was marked as outdated.

athowes commented Nov 13, 2024

seabbs commented Nov 13, 2024

seabbs commented Nov 14, 2024

seabbs left a comment

athowes left a comment •

edited

Loading

seabbs commented Nov 14, 2024

seabbs commented Nov 14, 2024

Issue #362: Reduce dependence on cmdstanr #424

Issue #362: Reduce dependence on cmdstanr #424

Conversation

athowes commented Nov 6, 2024 • edited by seabbs Loading

Description

Checklist

athowes commented Nov 11, 2024

This comment was marked as outdated.

athowes commented Nov 13, 2024

seabbs commented Nov 13, 2024

seabbs commented Nov 14, 2024

seabbs left a comment

Choose a reason for hiding this comment

athowes left a comment • edited Loading

Choose a reason for hiding this comment

seabbs commented Nov 14, 2024

seabbs commented Nov 14, 2024

Issue #362: Reduce dependence on `cmdstanr` #424

Issue #362: Reduce dependence on `cmdstanr` #424

athowes commented Nov 6, 2024 •

edited by seabbs

Loading

athowes left a comment •

edited

Loading