QInfer / python-qinfer

Library for Bayesian inference via sequential Monte Carlo for quantum parameter estimation.
BSD 3-Clause "New" or "Revised" License
92 stars 31 forks source link

Feature: Lui-West default number of particles #119

Closed ihincks closed 7 years ago

ihincks commented 7 years ago

This PR adds a parameter to the Lui-West resampler called default_n_particles which optionally stores a default number of particles to draw. Furthermore, SMCUpdater has the default behaviour of setting default_n_particles of the resampler to the __init__ input n_particles.

To the best of my knowledge, this should have no side-effects at present because (currently) the resampler is the only thing that changes the number of particles in the whole qinfer library.

The idea here is to allow canonicalize and update_timestep to drop particles (ala rejection filtering) without affecting the long-term stability. For example, if update_timestep takes particles outside of the allowed region, one of the most sensible things to do is drop them; trying to reflect them back in or something will probably have side-effects. Criticism of this PR welcome.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-11.4%) to 57.826% when pulling 25a00b96929cd7d6bdc5bc266b8e38b381df50d0 on ihincks:resampler-default-nparticles into 12b2612dfb0410acf6ee010eeb9d262b3914c201 on QInfer:master.

taalexander commented 7 years ago

The alternative to rejection filtering that may be used for canonicalize and update_timestep is postselection which is currently applied by the Liu-West resampler. Otherwise I do not see a downside to having an ability to set a default # of particles for resampling.

ihincks commented 7 years ago

Agreed, repeated sampling is an option for update_timestep and resampler. I think, however, that none of these solutions are ideal; I think the ideal route is to do what the big MCMC libraries do, and reparameterize your variable whenever it truly has bounded or semi-bounded support.

ihincks commented 7 years ago

How do I find out why the coverage went down? I only changed like 15 lines of code, a third of which were new documentation.

cgranade commented 7 years ago

I agree, resampling at a boundary does indicate a need for reparameterization. That said, rejection sampling in the manner of the current LiuWestResampler and using canonicalize seem to work well in a number of important cases, such that providing more tools to implement rejection sampling in a flexible manner is always a good thing. From that perspective, allowing resamplers to drop particles is a very good thing.

My only concern is with the increasing complexity of the API for resampling algorithms (much of that my fault, I'm afraid). I think there'd be some value in an eventual 2.0 release cleaning up some of my early mistakes in the design of resamplers, in particular reconciling the class hierarchy with Distribution such that the distinction between n_particles and default_n_particles would then either be on the call site (SMCUpdater), or included into the API for Distribution.sample more generally. In the meantime, however, this looks like a good improvement over the current state of affairs.

I'll look into the Coveralls problem quick before merging. Thanks!

cgranade commented 7 years ago

I think that #121 should fix the Coveralls issue. I'm still not sure why it only affects some of the subbuilds for each Travis build, but at the least we should get much better diagnostics over why Coveralls thinks the coverage went down.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.6%) to 71.756% when pulling 06a1018244b9cce996c01fe8de9475b6c6eaf479 on ihincks:resampler-default-nparticles into 12b2612dfb0410acf6ee010eeb9d262b3914c201 on QInfer:master.

ihincks commented 7 years ago

Thanks for figuring that out.