Skip to content

Regenerate draw ids for bind_draws(<draws_rvar>, along = "chain") - #306

Merged
paul-buerkner merged 5 commits into
masterfrom
issue-300
Oct 31, 2023
Merged

paul-buerkner merged 5 commits into
masterfrom
issue-300

Conversation

@mjskay

@mjskay mjskay commented Oct 31, 2023

Copy link
Copy Markdown
Collaborator

Summary

This PR closes #300. The underlying issue in #300 is that bind_draws(<draws_rvars>, along = "chain") did not re-generate draw ids, so the resulting object from split_chain(<draws_rvars>) (which uses bind_draws() could have multiple draws with th same id.

Notably, this bug might have been caught more easily if there was a warning issued by order_draws(<rvar>) if it needs to merge chains. I added a simple warning using warn_merge_chains(), but with the default settings that warning is not issued anyway. So I'm not sure if we want some other long-term solution, maybe warning levels or something?

Copyright and Licensing

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

@codecov-commenter

codecov-commenter commented Oct 31, 2023 •

Copy link
Copy Markdown

Codecov Report

Merging #306 (e80ce44) into master (7868fcf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master     #306   +/-   ##
=======================================
  Coverage   95.60%   95.60%           
=======================================
  Files          47       47           
  Lines        3683     3686    +3     
=======================================
+ Hits         3521     3524    +3     
  Misses        162      162           
Files Coverage Δ
R/bind_draws.R 97.00% <100.00%> (+0.03%) ⬆️
R/order_draws.R 100.00% <100.00%> (ø)

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

@github-actions

Copy link
Copy Markdown

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

  •   :ballot_box_with_check:as_draws_array: 181ms -> 177ms [-4.95%, +0.94%]
  •   :ballot_box_with_check:as_draws_df: 61.3ms -> 62.7ms [-4.47%, +9.26%]
  •   :ballot_box_with_check:as_draws_list: 321ms -> 324ms [-0.56%, +2.64%]
  •   :ballot_box_with_check:as_draws_matrix: 58.1ms -> 56.9ms [-6.28%, +2.25%]
  •   :ballot_box_with_check:as_draws_rvars: 291ms -> 296ms [-0.52%, +3.94%]
  •   :ballot_box_with_check:summarise_draws_100_variables: 1.28s -> 1.26s [-3.88%, +0.67%]
  •   :ballot_box_with_check:summarise_draws_10_variables: 139ms -> 139ms [-2.53%, +3.68%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@paul-buerkner

paul-buerkner commented Oct 31, 2023 •

Copy link
Copy Markdown
Collaborator

Thank you very much! A long term solution of warning levels sounds sensible. For this, I guess, we have to gather all potential warnings and decide which to issue by default and which to issue at different levels of complaininess. Anyway, for now, I will merge this PR, run all posterior and then release a new version to CRAN today.

@paul-buerkner
paul-buerkner merged commit 5e0a269 into master Oct 31, 2023
@mjskay
mjskay deleted the issue-300 branch October 31, 2023 15:26
@mjskay mjskay mentioned this pull request Nov 2, 2023
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.

subset_draws(x, chain = j) has incorrect behaviour following split_chains for type draws_rvars

3 participants