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

Pareto k tail fix #351

Merged
merged 4 commits into from
Mar 6, 2024
Merged

Conversation

n-kall
Copy link
Collaborator

@n-kall n-kall commented Mar 6, 2024

Summary

As brought up by @avehtari in issue #345, when one tail is constant, but tail = "both" is specified, it is better to return the diagnostic for the non-constant tail rather than NA.

Copyright and Licensing

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

@n-kall n-kall marked this pull request as draft March 6, 2024 10:12
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (5403ae5) to head (8435f4a).

❗ Current head 8435f4a differs from pull request most recent head 23d3e9a. Consider uploading reports for the commit 23d3e9a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
- Coverage   95.31%   95.29%   -0.02%     
==========================================
  Files          50       50              
  Lines        3840     3845       +5     
==========================================
+ Hits         3660     3664       +4     
- Misses        180      181       +1     

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

@n-kall n-kall marked this pull request as ready for review March 6, 2024 10:19
@paul-buerkner
Copy link
Collaborator

Looks sensible to me. Any more changes you want to do before I merge?

Copy link

github-actions bot commented Mar 6, 2024

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

  •   :ballot_box_with_check:as_draws_array: 144ms -> 144ms [-1%, +0.95%]
  • ❗🐌as_draws_df: 74.5ms -> 75.6ms [+0.77%, +2.09%]
  •   :ballot_box_with_check:as_draws_list: 178ms -> 178ms [-1.84%, +1.1%]
  • ❗🐌as_draws_matrix: 62.4ms -> 63.7ms [+0.55%, +3.42%]
  •   :ballot_box_with_check:as_draws_rvars: 85.3ms -> 86.1ms [-0.27%, +2.18%]
  •   :ballot_box_with_check:summarise_draws_100_variables: 709ms -> 707ms [-0.63%, +0.1%]
  •   :ballot_box_with_check:summarise_draws_10_variables: 111ms -> 111ms [-0.82%, +1.43%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@n-kall
Copy link
Collaborator Author

n-kall commented Mar 6, 2024

I don't have more to add regarding this, you can merge

@paul-buerkner paul-buerkner merged commit d0deed3 into stan-dev:master Mar 6, 2024
10 checks passed
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.

3 participants