Exclude draws #336

n-kall · 2024-01-11T11:08:37Z

Summary

Adds an exclude option to subset_draws that will remove the matched draws. Addresses #333

TODO:

~~Tests~~

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/)

codecov-commenter · 2024-01-11T11:20:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (627d2e8) 95.00% compared to head (aafaf7d) 95.15%.
Report is 4 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage   95.00%   95.15%   +0.15%     
==========================================
  Files          47       47              
  Lines        3744     3757      +13     
==========================================
+ Hits         3557     3575      +18     
+ Misses        187      182       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-01-11T11:46:04Z

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

:ballot_box_with_check:as_draws_array: 105ms -> 99.9ms [-15.58%, +5.36%]
❗🐌as_draws_df: 34.6ms -> 80.6ms [+131.02%, +135.52%]
:ballot_box_with_check:as_draws_list: 182ms -> 181ms [-0.87%, +0.37%]
:rocket:as_draws_matrix: 30.2ms -> 29.4ms [-4.33%, -0.4%]
:ballot_box_with_check:as_draws_rvars: 154ms -> 155ms [-1.13%, +1.81%]
:ballot_box_with_check:summarise_draws_100_variables: 716ms -> 713ms [-0.99%, +0.17%]
:ballot_box_with_check:summarise_draws_10_variables: 78.8ms -> 78.8ms [-0.6%, +0.47%]
Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions · 2024-01-11T12:58:30Z

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

:ballot_box_with_check:as_draws_array: 98.7ms -> 98.9ms [-0.6%, +0.97%]
❗🐌as_draws_df: 31.3ms -> 79.8ms [+148.27%, +161.95%]
❗🐌as_draws_list: 180ms -> 184ms [+1.33%, +2.86%]
:ballot_box_with_check:as_draws_matrix: 28.7ms -> 29.2ms [-2.35%, +5.97%]
:ballot_box_with_check:as_draws_rvars: 153ms -> 152ms [-2.11%, +1.17%]
:ballot_box_with_check:summarise_draws_100_variables: 710ms -> 711ms [-0.83%, +1.1%]
:ballot_box_with_check:summarise_draws_10_variables: 79ms -> 78ms [-3.37%, +0.64%]
Further explanation regarding interpretation and methodology can be found in the documentation.

n-kall · 2024-01-11T13:46:12Z

I tried to change as little code as necessary to implement this. But I think this is now ready for review

paul-buerkner

Thank you for working on this. I have left a few rather small comments I think

paul-buerkner · 2024-01-11T13:56:52Z

R/subset_draws.R

+#'   iterations, or draws be allowed? If `TRUE` (the default) only
+#'   unique chains, iterations, and draws are selected regardless of
+#'   how often they appear in the respective selecting arguments.
+#' @param exclude (logical) Should the selected subset be excluded?


It is not just selection of draws but of variables etc. too.

paul-buerkner · 2024-01-11T13:57:10Z

R/subset_draws.R

@@ -55,6 +61,22 @@ subset_draws.draws_matrix <- function(x, variable = NULL, iteration = NULL,
  iteration <- check_iteration_ids(iteration, x, unique = unique)
  chain <- check_chain_ids(chain, x, unique = unique)
  draw <- check_draw_ids(draw, x, unique = unique)
+
+  if (exclude) {


please input check exclude via as_one_logical

Thanks! Keep forgetting to do this. I now added input checking into the check_* functions, as is done with the the other arguments regex and unique. This does mean that the inputs are not checked if all draw, iteration, variable, chain are NULL though

paul-buerkner · 2024-01-11T13:58:30Z

R/subset_draws.R

@@ -55,6 +61,22 @@ subset_draws.draws_matrix <- function(x, variable = NULL, iteration = NULL,
  iteration <- check_iteration_ids(iteration, x, unique = unique)
  chain <- check_chain_ids(chain, x, unique = unique)
  draw <- check_draw_ids(draw, x, unique = unique)
+
+  if (exclude) {


given that this code needs to be repeated for every draws format? Should we perhaps have exclude handled inside check_existing_variables, check_iteration_ids etc? I think this would be cleaner actually

Good point, I changed this

github-actions · 2024-01-11T14:35:03Z

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

:ballot_box_with_check:as_draws_array: 100ms -> 100ms [-0.21%, +0.98%]
❗🐌as_draws_df: 35.2ms -> 82.7ms [+130.4%, +139.98%]
:ballot_box_with_check:as_draws_list: 182ms -> 181ms [-1.58%, +0.1%]
:ballot_box_with_check:as_draws_matrix: 30.7ms -> 29.6ms [-7.62%, +0.26%]
:ballot_box_with_check:as_draws_rvars: 156ms -> 156ms [-1.25%, +1.48%]
:ballot_box_with_check:summarise_draws_100_variables: 714ms -> 711ms [-1.21%, +0.2%]
:ballot_box_with_check:summarise_draws_10_variables: 78.8ms -> 79ms [-0.42%, +0.93%]
Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions · 2024-01-12T09:00:43Z

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

:ballot_box_with_check:as_draws_array: 101ms -> 100ms [-4.9%, +2.61%]
❗🐌as_draws_df: 36.8ms -> 82.4ms [+95.73%, +151.47%]
:ballot_box_with_check:as_draws_list: 184ms -> 183ms [-2.18%, +0.5%]
:ballot_box_with_check:as_draws_matrix: 29.8ms -> 29.4ms [-5.26%, +2.1%]
:ballot_box_with_check:as_draws_rvars: 157ms -> 155ms [-2.89%, +0.57%]
:rocket:summarise_draws_100_variables: 719ms -> 714ms [-1.13%, -0.19%]
:ballot_box_with_check:summarise_draws_10_variables: 78.6ms -> 78.5ms [-0.7%, +0.44%]
Further explanation regarding interpretation and methodology can be found in the documentation.

github-actions · 2024-01-12T10:20:10Z

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

:ballot_box_with_check:as_draws_array: 100ms -> 100ms [-1.4%, +1.51%]
❗🐌as_draws_df: 31.4ms -> 81ms [+153.84%, +161.5%]
:ballot_box_with_check:as_draws_list: 184ms -> 183ms [-2.28%, +0.98%]
:ballot_box_with_check:as_draws_matrix: 28.9ms -> 28.8ms [-2.89%, +2.09%]
:ballot_box_with_check:as_draws_rvars: 155ms -> 154ms [-2.93%, +1.04%]
:ballot_box_with_check:summarise_draws_100_variables: 718ms -> 717ms [-0.91%, +0.71%]
❗🐌summarise_draws_10_variables: 78.3ms -> 78.8ms [+0.01%, +1.21%]
Further explanation regarding interpretation and methodology can be found in the documentation.

n-kall · 2024-01-15T08:55:24Z

@paul-buerkner I think I addressed your comments now, also added more tests

paul-buerkner · 2024-01-15T15:17:14Z

Perfect. Thank you!

n-kall added 3 commits January 9, 2024 14:34

Merge branch 'master' of github.com:n-kall/posterior

12b0077

Merge branch 'master' of github.com:n-kall/posterior

4f4613c

add exclude argument to subset_draws

43e6b4b

n-kall added 2 commits January 11, 2024 14:27

fix handling of reserved variables when subsetting with exclude

e00566c

add tests for subset_draws with exclude = TRUE

95c1f01

n-kall added 2 commits January 11, 2024 15:38

improve subset_draws exclude documentation

e1624cb

documentation update

f46f75b

paul-buerkner requested changes Jan 11, 2024

View reviewed changes

n-kall added 2 commits January 12, 2024 10:51

move excluding to check_* functions

eab298d

modify subset_draws exclusion doc

2bcd48f

n-kall added 2 commits January 12, 2024 12:13

add more tests for subset with exclude

3905aa1

improve doc for subset with exclude

f919ba1

paul-buerkner merged commit a63b894 into stan-dev:master Jan 15, 2024
9 of 10 checks passed

n-kall mentioned this pull request Jan 16, 2024

should remove_variables be exported? #333

Closed

n-kall deleted the exclude_draws branch January 16, 2024 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude draws #336

Exclude draws #336

n-kall commented Jan 11, 2024 •

edited

Loading

codecov-commenter commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 11, 2024

n-kall commented Jan 11, 2024

paul-buerkner left a comment

paul-buerkner Jan 11, 2024

n-kall Jan 12, 2024

paul-buerkner Jan 11, 2024

n-kall Jan 12, 2024

paul-buerkner Jan 11, 2024

n-kall Jan 12, 2024

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

n-kall commented Jan 15, 2024

paul-buerkner commented Jan 15, 2024

Exclude draws #336

Exclude draws #336

Conversation

n-kall commented Jan 11, 2024 • edited Loading

Summary

Copyright and Licensing

codecov-commenter commented Jan 11, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 11, 2024

n-kall commented Jan 11, 2024

paul-buerkner left a comment

Choose a reason for hiding this comment

paul-buerkner Jan 11, 2024

Choose a reason for hiding this comment

n-kall Jan 12, 2024

Choose a reason for hiding this comment

paul-buerkner Jan 11, 2024

Choose a reason for hiding this comment

n-kall Jan 12, 2024

Choose a reason for hiding this comment

paul-buerkner Jan 11, 2024

Choose a reason for hiding this comment

n-kall Jan 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

n-kall commented Jan 15, 2024

paul-buerkner commented Jan 15, 2024

n-kall commented Jan 11, 2024 •

edited

Loading

codecov-commenter commented Jan 11, 2024 •

edited

Loading