add set_variables alias of variables<- #296

n-kall · 2023-08-01T09:27:52Z

Summary

Adds set_variables function which is an alias of variables<-. Fixes #281

Example functionality:

x <- as_draws(matrix(rnorm(100), ncol = 2))
variables(x)
#> [1] "...1" "...2"

x |> set_variables(c("theta[1]", "theta[2]")) |> variables()
#> [1] "theta[1]" "theta[2]"

# this is equivalent to
variables(x) <- c("theta[1]", "theta[2]")
variables(x)
#> [1] "theta[1]" "theta[2]"

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

paul-buerkner · 2023-08-01T09:39:33Z

Thank you! Would you mind moving the code to the R file where variables() <- is defined to keep them together? Same with the tests.

n-kall · 2023-08-01T10:00:53Z

``

Would you mind moving the code to the R file where variables() <- is defined to keep them together? Same with the tests.

Done!

paul-buerkner

Thank you! I have a few minor comments before merging this.

paul-buerkner · 2023-08-01T12:07:02Z

R/draws-index.R

+#' x <- as_draws(matrix(rnorm(100), ncol = 2))
+#' variables(x)
+#'
+#' x |> set_variables(c("theta[1]", "theta[2]")) |> variables()


Please avoid the use of the pipe in the docs because this may lead to a failur in old R versions not having the pipe.

paul-buerkner · 2023-08-01T12:07:30Z

R/draws-index.R

+#' variables(x)
+#'
+#' @export
+set_variables <- function(.x, ...) {


why use .x as the first argument here? Wouldn't x be okay too?

Fixed by changing variable to x

R/draws-index.R

mjskay · 2023-08-01T13:43:01Z

R/draws-index.R

+
+#' @rdname set_variables
+#' @export
+set_variables.draws <- function(x, variables, ...) {


Why make set_variables generic? If it is a non-generic function that just forwards to variables<- then implementers of other classes need only override the generic there.

I didn't think too much about this either way, as I just used rename_variables as a template, which is a generic with only a rename_variables.draws method defined. If it's better to change it to a non-generic function, I can do that

I defer to @paul-buerkner, but IMO it would be simpler :).

Good point. didn't think of this, but I agree. We should make it a non-generic function. Same with rename_variables I think but I will look at the latter separately.

non-generic sounds good to me too

I changed it to non-generic now

codecov-commenter · 2023-08-01T16:19:14Z

Codecov Report

Merging #296 (e4af9b4) into master (59f4e6c) will increase coverage by 0.11%.
Report is 27 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head e4af9b4 differs from pull request most recent head c74e765. Consider uploading reports for the commit c74e765 to get more accurate results

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   95.84%   95.96%   +0.11%     
==========================================
  Files          44       44              
  Lines        3347     3417      +70     
==========================================
+ Hits         3208     3279      +71     
+ Misses        139      138       -1

Files Changed	Coverage Δ
R/as_draws_df.R	`99.01% <100.00%> (ø)`
R/draws-index.R	`93.93% <100.00%> (+0.06%)`	⬆️
R/repair_draws.R	`98.14% <100.00%> (ø)`
R/rvar-.R	`97.50% <100.00%> (+0.36%)`	⬆️
R/rvar-apply.R	`100.00% <100.00%> (ø)`
R/rvar-cast.R	`98.96% <100.00%> (+0.09%)`	⬆️
R/rvar-dim.R	`100.00% <100.00%> (ø)`
R/rvar-rfun.R	`100.00% <100.00%> (ø)`
R/rvar-slice.R	`100.00% <100.00%> (ø)`
R/summarise_draws.R	`82.14% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2023-08-02T17:12:43Z

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

:ballot_box_with_check:as_draws_array: 158ms -> 156ms [-1.89%, +0.43%]
:rocket:as_draws_df: 124ms -> 54.1ms [-57.99%, -54.91%]
:ballot_box_with_check:as_draws_list: 278ms -> 272ms [-4.38%, +0.3%]
:rocket:as_draws_matrix: 48.8ms -> 47.9ms [-3.8%, -0.21%]
:ballot_box_with_check:as_draws_rvars: 258ms -> 256ms [-2.12%, +0.34%]
:ballot_box_with_check:summarise_draws_100_variables: 1.11s -> 1.11s [-1.3%, +0.49%]
:ballot_box_with_check:summarise_draws_10_variables: 122ms -> 122ms [-0.54%, +1.3%]
Further explanation regarding interpretation and methodology can be found in the documentation.

paul-buerkner

looks good. One minor thing before I will merge

paul-buerkner · 2023-08-04T12:08:01Z

R/draws-index.R

+#' x <- as_draws(matrix(rnorm(100), ncol = 2))
+#' variables(x)
+#'
+#' set_variables(x, c("theta[1]", "theta[2]"))


you are not assigning to x here. Is that intentional?

Good catch! Fixed this now.

github-actions · 2023-08-06T19:44:21Z

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

:ballot_box_with_check:as_draws_array: 146ms -> 151ms [-1.58%, +8.98%]
:rocket:as_draws_df: 138ms -> 51.2ms [-67.33%, -58.67%]
:ballot_box_with_check:as_draws_list: 264ms -> 272ms [-1.06%, +7.03%]
:ballot_box_with_check:as_draws_matrix: 46.7ms -> 45.8ms [-9.44%, +5.49%]
:ballot_box_with_check:as_draws_rvars: 255ms -> 250ms [-6.52%, +2.99%]
:ballot_box_with_check:summarise_draws_100_variables: 1.1s -> 1.1s [-2.49%, +1.4%]
:ballot_box_with_check:summarise_draws_10_variables: 122ms -> 121ms [-5.62%, +4.04%]
Further explanation regarding interpretation and methodology can be found in the documentation.

paul-buerkner · 2023-08-07T08:13:46Z

Thank you for adding this!

n-kall · 2023-08-07T08:23:03Z

Thank you for adding this!

Thanks for the review!

add set_variables alias of variables<- with documentation and tests

54c579b

n-kall changed the title ~~add set_variables alias of variables<- with documentation and tests~~ add set_variables alias of variables<- Aug 1, 2023

move set_variables to be in same files as variables function

d0d245f

paul-buerkner requested changes Aug 1, 2023

View reviewed changes

minor tweaks to set_variables

4c3de9e

mjskay reviewed Aug 1, 2023

View reviewed changes

n-kall added 2 commits August 1, 2023 19:15

change set_variables to non-generic function

754b47a

update documentation for set_variables

f74cdec

n-kall requested a review from paul-buerkner August 3, 2023 06:32

paul-buerkner requested changes Aug 4, 2023

View reviewed changes

fix example for set_variables

c74e765

paul-buerkner approved these changes Aug 6, 2023

View reviewed changes

paul-buerkner merged commit 55e9233 into stan-dev:master Aug 7, 2023
10 checks passed

n-kall deleted the set_variables branch August 7, 2023 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add set_variables alias of variables<- #296

add set_variables alias of variables<- #296

n-kall commented Aug 1, 2023

paul-buerkner commented Aug 1, 2023

n-kall commented Aug 1, 2023

paul-buerkner left a comment

paul-buerkner Aug 1, 2023

n-kall Aug 1, 2023

paul-buerkner Aug 1, 2023

n-kall Aug 1, 2023

mjskay Aug 1, 2023

n-kall Aug 1, 2023

mjskay Aug 1, 2023

paul-buerkner Aug 1, 2023

jgabry Aug 1, 2023

n-kall Aug 1, 2023

codecov-commenter commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023

paul-buerkner left a comment

paul-buerkner Aug 4, 2023

n-kall Aug 4, 2023

github-actions bot commented Aug 6, 2023

paul-buerkner commented Aug 7, 2023

n-kall commented Aug 7, 2023

add set_variables alias of variables<- #296

add set_variables alias of variables<- #296

Conversation

n-kall commented Aug 1, 2023

Summary

Copyright and Licensing

paul-buerkner commented Aug 1, 2023

n-kall commented Aug 1, 2023

paul-buerkner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 1, 2023 • edited Loading

Codecov Report

github-actions bot commented Aug 2, 2023

paul-buerkner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 6, 2023

paul-buerkner commented Aug 7, 2023

n-kall commented Aug 7, 2023

codecov-commenter commented Aug 1, 2023 •

edited

Loading