stan-dev / posterior

The posterior R package
https://mc-stan.org/posterior/
Other
167 stars 24 forks source link

extract_variable_array and the with_indices parameter for `variables()` #342

Closed mjskay closed 10 months ago

mjskay commented 10 months ago

Summary

This PR:

It also makes a few other related changes:

Copyright and Licensing

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

codecov-commenter commented 10 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0322b46) 95.14% compared to head (45b0643) 95.31%. Report is 28 commits behind head on master.

:exclamation: Current head 45b0643 differs from pull request most recent head fe6f23b. Consider uploading reports for the commit fe6f23b to get more accurate results

Files Patch % Lines
R/variables.R 99.06% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #342 +/- ## ========================================== + Coverage 95.14% 95.31% +0.17% ========================================== Files 47 50 +3 Lines 3745 3840 +95 ========================================== + Hits 3563 3660 +97 + Misses 182 180 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 10 months ago

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

github-actions[bot] commented 10 months ago

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

github-actions[bot] commented 10 months ago

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

paul-buerkner commented 10 months ago

thank you! Please let me know once this PR is ready for review!

mjskay commented 10 months ago

Should be ready now

avehtari commented 10 months ago

closes https://github.com/stan-dev/posterior/issues/340 by adding an extract_variable_array() function to extract variables with indices into arrays of iterations x chains x any remaining dimensions. @avehtari does this do what you need?

Yes, it's helpful as it works the same way for any draws object type, and I can write my example in the issue as

p1$draws() |> subset_draws(draw=1) |> extract_variable_array(variable='r_id') |> drop()

or if I want to get all draws array

p1$draws() |> merge_chains() |> extract_variable_array(variable='r_id') |> drop()

(I can also drop drop() and index the 4 dim array)

paul-buerkner commented 10 months ago

Great! @mjskay anything to change from your side? Otherwise, feel free to merge (or tell me such that I merge).

mjskay commented 10 months ago

Thanks for the quick review @paul-buerkner!

Maybe let's wait a day or two and see if @jgabry @MansMeg @avehtari or others have objections/suggestions for the with_indices parameter name, and if not then I can merge.

paul-buerkner commented 10 months ago

Agreed.

avehtari commented 10 months ago

I'm fine with with_indices and can't come up with better suggestions. I tested using variables(), nvariables(), and set_variables(), and having with_indices parameter is awesome!

mjskay commented 10 months ago

Great, thanks @avehtari! I will take that as a strong endorsement and merge :)