search

sblunt / orbitize

Orbit-fitting for directly imaged objects
https://orbitize.info
Other
79 stars 44 forks source link

'Walkers Outside Prior Support' error catching #251

Closed adj-smith closed 3 years ago

adj-smith commented 3 years ago

When starting an MCMC after manually placing your walker positions, it is possible to have walkers in locations that are disallowed by the prior. When this happens, ptemcee throws a rather unhelpful error message. (ValueError: Attempting to start with samples outside posterior support.) Since the walker positions and priors are manipulated by orbitize! already, it would be convenient for orbitize.sampler.run_sampler() to detect these issues, stop itself, and throw a more helpful error message, prior to going to ptemcee.

I'm already starting to plan out how this might be accomplished, but I don't have enough knowledge of the inner workings of the modules yet to implement. If I had a little guidance, it should be possible to prototype something fairly quickly. My plan:

1) Add an is_supported() method to the prior classes. Essentially, this would simply compare the passed variable's value against the prior's conditions and return a boolean. For some priors (e.g. Gaussian with no_negatives == False) this would of course always return True. 2) Add a check_prior_support() method to the sampler.MCMC class. This would consist of a loop that, for each walker, compares the values of each parameter against the appropriate prior by calling prior.is_supported(). This method would return a boolean and a list; the boolean will be True if so long as all prior.is_supported() calls also return True. If any return False, check_prior_support() will return False and the bad parameter(s)' index(es) will be returned in the list. 3) Add a line to MCMC.run_sampler(): 

if not self.check_prior_support():
    raise ValueError( 'Attempting to start with parameter(s) outside prior support:'+','.join(BadParameterList) )

I'm happy to work on implementing this sort of feature. Guidance about the structure of prior and sampler.MCMC would be appreciated, to minimize how much I accidentally reinvent the wheel, if anyone can offer it. By eye it seems small enough that I could have a prototype done by Aug 20, if not before; if it takes longer than that, progress will slow down due to other obligations on my end (classes).

sblunt commented 3 years ago

That would be great, Adam. This is the same as issue #178, so check out the conversation there before you get started. I don't think step 1 is necessary-- you could just add a line within the check_prior_support() function you describe in step 2 that checks that the prior lnlike values are finite. It would be a lot of repeated code to add that one line to every Prior object individually.

sblunt commented 3 years ago

Thanks for bringing this up and offering to take it on!

adj-smith commented 3 years ago

Thanks for the advice @sblunt - I was able implement without the first step I described by following your suggestion. It saved me some time and effort. I'm still running tests to make sure there's no bugs (and that I can't shave the time down a little more) but I have a functioning prototype now.

adj-smith commented 3 years ago

My code has passed all the tests I could think of, and is as compact as I know how to make it at this time. It correctly and quickly (avg runtime 1~1.5 ms) identifies any parameters that return a non-finite lnlike from the correct Prior object, and reports a ValueError listing which (if any) parameter indices caused issues. It also correctly fails to raise the error if no parameters are incorrectly placed.

Everything is contained in one function, which is called directly from run_sampler(); currently, I have this call placed near the top of that function, but so long as it gets called before sampler.sample(), it should do what is intended.

Comments:

I've created a pull request for this change.

semaphoreP commented 3 years ago

Implemented in PR #253 🎉 Thanks @adj-smith!