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

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

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

mjskay
Copy link
Collaborator

@mjskay mjskay commented Oct 31, 2023

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
Copy link

codecov-commenter commented Oct 31, 2023

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

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
Copy link
Collaborator

paul-buerkner commented Oct 31, 2023

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
9 of 10 checks passed
@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