-
Notifications
You must be signed in to change notification settings - Fork 220
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
Bump AbstractMCMC major version #1579
Conversation
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, just have to make sure that it's actually tested and needs a version bump 👍
The same strange errors in the Gibbs MH test. I'm even more surprised that the errors occur on Julia 1.3 🤔 |
The failing tests were added recently in #1557. |
@cpfiffer Maybe we should just change the test in Turing.jl/test/inference/mh.jl Line 137 in 2eab753
|
But it should be linking, no? |
At least that's the intention: Line 404 in 2eab753
|
I think I see the issue. https://github.com/TuringLang/Turing.jl/blob/master/src/inference/mh.jl#L404 passes |
Pull Request Test Coverage Report for Build 736956259
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #1579 +/- ##
=======================================
Coverage 78.16% 78.16%
=======================================
Files 23 23
Lines 1429 1429
=======================================
Hits 1117 1117
Misses 312 312 Continue to review full report at Codecov.
|
@@ -32,7 +32,7 @@ Tracker = "9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c" | |||
ZygoteRules = "700de1a5-db45-46bc-99cf-38207098b444" | |||
|
|||
[compat] | |||
AbstractMCMC = "2.1" | |||
AbstractMCMC = "2.1, 3" |
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.
AbstractMCMC = "2.1, 3" | |
AbstractMCMC = "3" |
This should cut out AbstractMCMC 2 because the internal API 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.
But this is only relevant for users that use custom callbacks? I.e., for Turing it should be fine to support both versions since we do not depend on these internals, do we?
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.
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.
Yeah, leaving it as is:)
@@ -28,7 +28,7 @@ Tracker = "9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c" | |||
Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f" | |||
|
|||
[compat] | |||
AbstractMCMC = "2.1" | |||
AbstractMCMC = "2.1, 3" |
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.
AbstractMCMC = "2.1, 3" | |
AbstractMCMC = "3" |
Same here.
Supersedes #1574 and #1575