-
Notifications
You must be signed in to change notification settings - Fork 44
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
Remove SimpleUnpack #401
Remove SimpleUnpack #401
Conversation
ext/AdvancedHMCMCMCChainsExt.jl
Outdated
param_names = missing, | ||
bijector = identity, | ||
discard_initial=0, | ||
thinning=1, |
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.
@ErikQQY, consider avoiding these styling changes in PRs. It's okay to do it via a separate PR if we decide to follow a different style.
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.
I think autoformatting would make these same changes if we copied the .JuliaFormatter.toml
from here: https://github.com/TuringLang/Turing.jl/blob/main/.JuliaFormatter.toml
I think that would be a good PR to make, but I agree that it should be a separate PR, because it's hard and a bit risky to read diffs that mix actual changes made by a human with autoformatting changes in unrelated parts of the codebase.
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, I was trying to format before commit habitually but found out the formatting style has not been configured yet.
julia = "1.6.7" | ||
LinearAlgebra = "1.10" | ||
Random = "1.10" | ||
julia = "1.10" |
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.
@devmotion, will dropping Julia's pre-1.10 support cause an issue?
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.
I'm fine with updating the min compat to 1.10. I don't see any need for it though - the package works fine on >= 1.6, and there's no add maintenance burden on < 1.10 currently. SimpleUnpack also just uses the Julia-native destructuring on Julia >= 1.7 (where it was introduced).
@ErikQQY I'm happy with the changes. Can you separate the styling change and removal of |
Thanks @ErikQQY! |
Since there is a standard destructuring syntax in Julia, we can remove SimpleUnpack then.
This PR also set Julia compat to 1.10, together with LinearAlgebra, Random etc.