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

new Pareto-k diagnostics vignette #372

Merged
merged 5 commits into from
Jun 28, 2024
Merged

new Pareto-k diagnostics vignette #372

merged 5 commits into from
Jun 28, 2024

Conversation

avehtari
Copy link
Collaborator

Fixes #322

@avehtari avehtari mentioned this pull request Jun 27, 2024
Copy link

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

  • ✔️as_draws_array: 146ms -> 145ms [-1.37%, +0.64%]
  • 🚀as_draws_df: 71ms -> 68.3ms [-5.86%, -1.68%]
  • ✔️as_draws_list: 170ms -> 170ms [-0.96%, +0.77%]
  • ✔️as_draws_matrix: 28.2ms -> 28.3ms [-0.53%, +1.51%]
  • ✔️as_draws_rvars: 78.9ms -> 79.3ms [-0.25%, +1.11%]
  • ✔️summarise_draws_100_variables: 697ms -> 697ms [-0.18%, +0.2%]
  • ✔️summarise_draws_10_variables: 111ms -> 111ms [-1%, +1.13%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@paul-buerkner
Copy link
Collaborator

Thanks! Do you think this vignette needs additional review? If yes, could @n-kall do it since you have been implementing these methods.

@n-kall
Copy link
Collaborator

n-kall commented Jun 27, 2024

Sure, I can look at this

Copy link
Collaborator

@n-kall n-kall left a comment

Choose a reason for hiding this comment

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

I haven't yet had a chance to run the vignette, but as the tests were passing (except on R 3.6) I have gone through and made some suggestions.

Most suggestions are removing the |> operator which is not available in R < 4.1.0. If we want to use the native pipe, I think would need to bump the required version to 4.1.0 (as far as I understand)

Comment on lines 43 to 48
dr<-array(data=replicate(4,as.numeric(arima.sim(n = N,
list(ar = c(phi)),
sd = sqrt((1-phi^2))))),
dim=c(N,4,1)) |>
as_draws_df() |>
set_variables('xn')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can use the |> operator unless we bump the required R version.

Instead this could be written

Suggested change
dr<-array(data=replicate(4,as.numeric(arima.sim(n = N,
list(ar = c(phi)),
sd = sqrt((1-phi^2))))),
dim=c(N,4,1)) |>
as_draws_df() |>
set_variables('xn')
dr <- array(data = replicate(4, as.numeric(arima.sim(n = N,
list(ar = c(phi)),
sd = sqrt((1 - phi^2))))),
dim = c(N, 4, 1))
dr <- as_draws_df(dr)
variables(dr) <- "xn"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As posterior suggests dplyr, can I use %>%?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think so

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. dplyr is importing magrittr so should work. That will be a much better solution

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to %>%

vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
We examine the draws with the default `summarise_draws()`.

```{r summarise_draws}
drt |> summarise_draws()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
drt |> summarise_draws()
summarise_draws(drt)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better here to use default_convergence_measures()to focus on the convergence diagnostics at first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like it as it is now

vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Outdated Show resolved Hide resolved
vignettes/pareto_diagnostics.Rmd Show resolved Hide resolved
Copy link

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

  • ✔️as_draws_array: 137ms -> 137ms [-0.8%, +1.11%]
  • ✔️as_draws_df: 65ms -> 64.1ms [-3.1%, +0.31%]
  • 🚀as_draws_list: 163ms -> 161ms [-1.97%, -0.4%]
  • ✔️as_draws_matrix: 27.6ms -> 27.7ms [-0.34%, +1.4%]
  • ✔️as_draws_rvars: 80.2ms -> 81.2ms [-0.63%, +2.99%]
  • ✔️summarise_draws_100_variables: 696ms -> 698ms [-0.09%, +0.51%]
  • ✔️summarise_draws_10_variables: 110ms -> 110ms [-0.69%, +1.21%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@avehtari
Copy link
Collaborator Author

Thanks Noa.

I made the suggested, except for using %>% and not using default_convergence_measures().

I also fixed the two references in posterior.Rmd. Could have been a separate from this PR, but maybe this is ok

vignettes/posterior.Rmd Outdated Show resolved Hide resolved
Copy link

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

  • ✔️as_draws_array: 142ms -> 143ms [-0.3%, +1.77%]
  • 🚀as_draws_df: 65.3ms -> 63ms [-4.56%, -2.66%]
  • ✔️as_draws_list: 158ms -> 157ms [-1.15%, +0.65%]
  • ✔️as_draws_matrix: 27.3ms -> 27.4ms [-0.68%, +0.94%]
  • ✔️as_draws_rvars: 79.8ms -> 79.5ms [-1.55%, +0.74%]
  • ✔️summarise_draws_100_variables: 694ms -> 694ms [-0.2%, +0.2%]
  • ✔️summarise_draws_10_variables: 110ms -> 110ms [-1.09%, +0.29%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

Co-authored-by: Noa Kallioinen <[email protected]>
@avehtari
Copy link
Collaborator Author

Thanks Noa, you have magical eyes for spotting errors!

Copy link

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

  • ✔️as_draws_array: 135ms -> 135ms [-1.32%, +1.17%]
  • 🚀as_draws_df: 70.5ms -> 68.3ms [-5.29%, -0.96%]
  • 🚀as_draws_list: 162ms -> 160ms [-2.3%, -0.06%]
  • ✔️as_draws_matrix: 27ms -> 26.7ms [-2.23%, +0.61%]
  • ✔️as_draws_rvars: 80.9ms -> 80.8ms [-1.34%, +1.08%]
  • ✔️summarise_draws_100_variables: 675ms -> 678ms [-0.5%, +1.45%]
  • ✔️summarise_draws_10_variables: 115ms -> 115ms [-1.11%, +1.21%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@n-kall
Copy link
Collaborator

n-kall commented Jun 28, 2024

Looks good! All the vignettes are failing to build with R 3.6 but I think that is due to the knitr / evaluate.
I think this could be merged

@avehtari avehtari merged commit 470075c into master Jun 28, 2024
9 of 11 checks passed
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.

Vignette for Pareto diagnostics
3 participants