Make MALA allow repeated ask() #994
Conversation
Codecov Report
@@ Coverage Diff @@
## master #994 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 84 63 -21
Lines 8823 6277 -2546
===========================================
- Hits 8823 6277 -2546
Continue to review full report at Codecov.
|
ben18785
left a comment
ben18785
left a comment
There was a problem hiding this comment.
Like the clean up of the tests, thanks. Not sure I follow the reasons for removing the checks for repeated asks (seeing as I don't see when someone would want to do this)?
pints/_mcmc/_mala.py
Outdated
|
|
||
| if self._ready_for_tell: | ||
| raise RuntimeError('Ask() called when expecting call to tell().') | ||
|
|
||
| # Propose new point |
There was a problem hiding this comment.
Not sure I follow why we'd want to remove this check? Probably missing the point, but what would be a use case for repeatedly calling ask?
| # Check if we had a proposal | ||
| if not self._ready_for_tell: | ||
| if self._proposed is None: | ||
| raise RuntimeError('Tell called before proposal was set.') | ||
| self._ready_for_tell = False | ||
|
|
There was a problem hiding this comment.
Sorry, probably not getting something but don't quite get why we'd remove these checks.
There was a problem hiding this comment.
It's more in line with what we have in the other MCMC methods, with the exception of the ones that do more complicated stuff in ask() than just set a proposal.
So no very strong reason! Just that it's possible to let users ask() as often as they like in this case (and always get the same result), and it lets us remove a variable...
We might want to disallow this everywhere instead though? In which case we'd have to update the documentation for SingleChainMCMC and MultiChainMCMC a bit to make this explicit...
|
Cool. Not sure about this one currently, to be honest. Let me think about
it...
…On Wed, Oct 9, 2019 at 12:17 AM Michael Clerx ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pints/_mcmc/_mala.py
<#994 (comment)>:
> - if not self._ready_for_tell:
+ if self._proposed is None:
raise RuntimeError('Tell called before proposal was set.')
- self._ready_for_tell = False
It's more in line with what we have in the other MCMC methods, with the
exception of the ones that do more complicated stuff in ask() than just set
a proposal.
So no very strong reason! Just that it's possible to let users ask() as
often as they like in this case (and always get the same result), and it
lets us remove a variable...
We might want to disallow this everywhere instead though? In which case
we'd have to update the documentation for SingleChainMCMC and
MultiChainMCMC a bit to make this explicit...
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#994?email_source=notifications&email_token=ABCILKGLYVAHP6GNXOQGGDLQNUIKBA5CNFSM4I6XHKBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHJ6VDY#discussion_r332773584>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABCILKFFBVMNY46K47IUA3DQNUIKBANCNFSM4I6XHKBA>
.
|
|
Made a ticket here #996 |
|
Can we re-open discussion on this, @ben18785 and @martinjrobins ? See especially #996 |
|
No thoughts on this?
|
See #996
There's no reason not!
Also cleaned up the tests a bit