-
Notifications
You must be signed in to change notification settings - Fork 6
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
Change dimension ordering #50
Conversation
Necessary because chain_inds are not identical to those in the previous example (now repeating with inner instead of outer)
Pull Request Test Coverage Report for Build 3680309266Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Codecov ReportBase: 94.41% // Head: 94.50% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 94.41% 94.50% +0.09%
==========================================
Files 9 9
Lines 609 619 +10
==========================================
+ Hits 575 585 +10
Misses 34 34
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I opted to wait to allow |
Bump @devmotion do you think you will have time to review this soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @sethaxen, I missed that the PR was waiting for my review. Looks mostly good 👍 I had some minor comments and questions but there shouldn't be any major issues.
Did you rerun #49 (comment) with the updated ordering?
Co-authored-by: David Widmann <[email protected]>
@devmotion I have implemented all requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
Pkg.develop(; path=dirname(dirname(pathof(MCMCDiagnosticTools)))) | ||
Pkg.instantiate() | ||
include(joinpath("rstar", "runtests.jl")) | ||
if Sys.WORD_SIZE == 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we do not only test XGBoost but also e.g. SVC nowadays maybe this check could be moved to rstar.jl and only XGBoost could be disabled. Not part of this PR though, and I guess nobody cares since we only run tests on 64bit anyway.
Co-authored-by: David Widmann <[email protected]>
Changes dimension ordering for 3d arrays to be
(nparams, ndraws, nchains)
(ndraws, nchains, nparams)
andadds method with 3d array inputs foras proposed in #49.mcse