pints icon indicating copy to clipboard operation
pints copied to clipboard

Ask-tell: Be explicit about allowed number of asks()

Open MichaelClerx opened this issue 6 years ago • 9 comments

Currently the MCMC methods operate in one of two ways:

Form 1:

def ask():
    if self._proposed is None:
        self._proposed = ...
    return self._proposed

def tell(fx):
    if self._proposed is None:
        raise RuntimeError('No proposal set / expecting ask')
    ...
    self._proposed = None

In this method you can call ask() as often as you like between tells, it will just return the same proposal again and again and again. So ask tell ask tell is the same as ask ask ask tell ask ask tell ask ask. Two tells are never allowed ask tell tell = error.

Form 2:

def ask():
    if self._ready_for_tell:
        raise RuntimeError('Expecting tell()')
    do_complex_stuff()
    set_a_proposal()
    self._ready_for_tell = True

def tell():
    if not self._ready_for_tell:
        raise RuntimeError('Expecting ask()')
    self._ready_for_tell = False

In this method you're only allowed one ask() per tell. So ask ask tell is an error, as is ask tell tell.

So what

Form 2 is easier for complex methods. And we don't have a use case where form 1 is really necessary. So... should we explicitly disallow form 1?

@ben18785 @martinjrobins ?

MichaelClerx avatar Oct 08 '19 23:10 MichaelClerx

Thoughts @martinjrobins ?

MichaelClerx avatar Nov 26 '19 13:11 MichaelClerx

See also #994

MichaelClerx avatar Nov 26 '19 13:11 MichaelClerx

Yes, I say disallow 1, ask is always followed by tell, which is always followed by ask. This should be documented in the base classes, but I don't think it matters wether individual methods allow you do do 1 though, as long as they support 2

martinjrobins avatar Apr 24 '20 11:04 martinjrobins

Cool. @chonlei and @ben18785 do you agree?

MichaelClerx avatar Apr 24 '20 11:04 MichaelClerx

If we go ahead with this, it would involve

  • [ ] Update the documentation for the ask() method in the base classes for optimisation, mcmc, nested, and abc (and any others?)
  • [ ] ~Update the tests for all methods. Some already have a check, I think, if not add in a check that a repeated ask raises the correct exception~
  • [ ] ~Fix all methods that fail the tests~
  • [ ] Check that no method has extra code to allow repeated asks. If allowed repeated asks follow naturally that's ok.

MichaelClerx avatar Apr 24 '20 11:04 MichaelClerx

as I said, I don't think that methods that allow you do repeated asks should be flagged as needing to be fixed. I can't see the harm in allowing it, and its a lot more code to write and maintain in explicitly disallowing it

martinjrobins avatar Apr 24 '20 11:04 martinjrobins

It could just give people the wrong impression, is what I'm thinking. They might write code that does repeated asks and then a while later change the method and report a 'bug'

MichaelClerx avatar Apr 24 '20 11:04 MichaelClerx

agree that we still need the documentation in the ask/tell methods to make this clear

martinjrobins avatar Apr 24 '20 12:04 martinjrobins

Alright, I can live with that

MichaelClerx avatar Apr 24 '20 12:04 MichaelClerx