Skip to content

split r-hat - #37

Merged
WardBrian merged 5 commits into
mainfrom
rank-split-r-hat
Jul 7, 2023
Merged

split r-hat#37
WardBrian merged 5 commits into
mainfrom
rank-split-r-hat

Conversation

@bob-carpenter

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

codecov-commenter commented Jun 28, 2023

Copy link
Copy Markdown

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
Copy Markdown
Collaborator Author

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

@WardBrian WardBrian left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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.

Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread test/test_rhat.py Outdated
Comment thread test/test_rhat.py Outdated
Comment thread test/test_rhat.py Outdated
Comment thread test/test_rhat.py
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py
Comment thread bayes_kit/rhat.py
Comment thread bayes_kit/rhat.py
Comment thread bayes_kit/rhat.py Outdated
Comment thread bayes_kit/rhat.py
Comment thread test/test_rhat.py Outdated
Comment thread test/test_rhat.py
Comment thread test/test_rhat.py
@bob-carpenter

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

A few minor comments

27 of them and counting. :-(.

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

@WardBrian WardBrian left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread bayes_kit/rhat.py Outdated
@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.

4 participants