Skip to content

Conversation

@bob-carpenter
Copy link
Collaborator

I added

  • implementation of split-R-hat (with tests)
  • additional validation in basic R-hat to ensure each chain has 2 or more draws (I added an additional test for this)

This resolves issue #36.

@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2023

Codecov Report

Merging #37 (52a5086) into main (dff8143) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   97.91%   98.10%   +0.18%     
==========================================
  Files          11       11              
  Lines         288      316      +28     
==========================================
+ Hits          282      310      +28     
  Misses          6        6              
Impacted Files Coverage Δ
bayes_kit/rhat.py 100.00% <100.00%> (ø)

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

@bob-carpenter
Copy link
Collaborator Author

I can update for 100% coverage of the tests. And apply black.

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments

@bob-carpenter
Copy link
Collaborator Author

This is ready to review. Failures are all in MCMC tests, not in the rhat stuff I added.

This is all updated with 100% coverage tests. I just added split_rhat and rank_normalized_rhat to the same rhat.py file. The doc has full definitions and pointers to the original references.

@bob-carpenter
Copy link
Collaborator Author

@jsoules and @WardBrian. Thanks for all the comments. I'll learn Python one PR at a time.

I made all the fixes I could understand and marked them as resolved.

There are a couple points I didn't know how to fix and I would really appreciate guidance.

@bob-carpenter
Copy link
Collaborator Author

A few minor comments

27 of them and counting. :-(.

There are two remaining concerns that I do not know how to address:

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one typo, but I promise this is my last comment. Feel free to merge after fixing

@WardBrian WardBrian merged commit 3157c8f into main Jul 7, 2023
@WardBrian WardBrian deleted the rank-split-r-hat branch July 7, 2023 13:29
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.

5 participants