Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implementation of Robust Adaptive Metropolis #106
Implementation of Robust Adaptive Metropolis #106
Changes from 26 commits
85ec534
de519a4
40ebb7e
2dec18a
045f8c5
755a180
5c1c6f5
cddf8d1
29c9078
78a5f51
652a227
5eaff52
d8688fa
f5fc301
56ec717
da431b4
f2889a0
9247281
4764120
11f3b64
df4feb1
f784492
45820d2
7405a19
5dce265
5ee44e3
37a2189
5193119
d4a144e
6295e78
6f8fda4
5815a9b
1b38ca6
f426d0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
That looks like one backtick too many. PS. How do I make a GitHub suggestion for this?
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.
if you have N backticks surround them with N+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.
On second thought, I just pushed a fix for this myself.
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.
Thanks @penelopeysm. :) I should have guessed that the universe would have been organised in such a way that the fix to quadruple backticks would be more quadruple backticks.
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.
Maybe only bound it in the update of
S
? It seems at least easier to read if the bounding is kept together with the part of the algorithm where it's actually needed.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 IMO this makes it a bit strange if we then put the unbounded
logα
in the resulting state, since this is not the quantity used to update theS
😕And for the purposes it is user here, it doesn't actually matter if it's bounded or not, right? As in, it's equivalent here, but not equivalent in the
S
update, hence it seems somewhat natural for me to just do it once and for all.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.
It is, isn't it? The update is just slightly different. Otherwise with the same reasoning you could also argue that one should only store
α
(or maybe the difference to the targeted α?) since only these are used to update S.In the end I guess it doesn't matter as it's only used in these two places. It just felt strange conceptually to bound it here, in particular since it seemed you already felt the need to explain this decision with a comment.
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 alpha represents the acceptance probability, no? So clamping it like this is technically always what you should do, but most of time we don't because it's unnecessary for sampling according to this probability.
However, if the user wants to actually look at the resulting acceptance probs, then it's a question of: do we want the user to do
or
In my head, the user expects to do the former, not the latter 🤷
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 can see your point. Even though I think neither of the two alternatives is particularly user-friendly, IMO a separate API for acceptance probabilities would be better.
In any case, I think I wouldn't even have commented on this line if the comment
#
minbecause we'll use this for updating
would not have been there. So maybe just remove the comment?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.
Fair, but then I'm worried someone might come along later and go "wait, that's not needed; let's just remove this unnecessary
min
", not realizing that we'll use this for adaptation 😬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.
Maybe add an
@assert logα <= zero(logα)
at the top ofram_adapt
?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.
Has JuliaFormatter been run? Just wondering because I thought the style guide was to not have whitespace around kwarg defaults. Not important though, can be left as is.